Skip to content

chore(typescript): Switch to --moduleResolution bundler, remove duplicate properties, remove @ts-expect-error comments #91686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DanielRosenwasser
Copy link
Contributor

@DanielRosenwasser DanielRosenwasser commented May 14, 2025

Related to #90413.

In order to cleanly measure relative performance improvements in the native port of the TypeScript compiler, this change removes some errors found by compiling with tsgo. Specifically:

  • duplicate computed-name properties have been removed
  • unnecessary @ts-expect-error comments have been removed (thanks to new moduleResolution settings`)
  • deep imports into @emotion/styled have been replaced by updating an existing top-level import
  • module resolution has been updated to bundler because
    • --moduleResolution node a.k.a. node10 will be deprecated in TypeScript 6.0
    • --moduleResolution nodenext will not work because many existing paths in import() calls lack a file extension
    • in reality, most files are processed by a bundler down the line (or a runtime like ts-node that has bundler-like resolution mechanics)

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@DanielRosenwasser DanielRosenwasser requested review from a team as code owners May 14, 2025 22:48
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 14, 2025
@DanielRosenwasser DanielRosenwasser marked this pull request as draft May 14, 2025 23:05
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 14, 2025
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

@DanielRosenwasser DanielRosenwasser changed the title chore(typescript): Switch to moduleResolution: nodenext, remove duplicate properties chore(typescript): Switch to --moduleResolution bundler, remove duplicate properties, remove @ts-ignore comments May 14, 2025
@DanielRosenwasser DanielRosenwasser changed the title chore(typescript): Switch to --moduleResolution bundler, remove duplicate properties, remove @ts-ignore comments chore(typescript): Switch to --moduleResolution bundler, remove duplicate properties, remove @ts-expect-error comments May 14, 2025
@DanielRosenwasser DanielRosenwasser marked this pull request as ready for review May 14, 2025 23:41
@ryan953 ryan953 requested a review from scttcper May 15, 2025 03:09
@ryan953 ryan953 added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label May 15, 2025
@ryan953
Copy link
Member

ryan953 commented May 15, 2025

@DanielRosenwasser I'm seeing the following output, but its obvs set to be "module": "preserve" & "moduleResolution": "bundler" in the PR. I'll look deeper in the morning to make sure that the PR branch changes are being picked up and applied in CI.

$ node scripts/test.js --ci --maxWorkers=100% --colors --forceExit
Error: Jest: Failed to parse the TypeScript config file /home/runner/work/sentry/sentry/jest.config.ts
  TSError: ⨯ Unable to compile TypeScript:
error TS5095: Option 'bundler' can only be used when 'module' is set to 'preserve' or to 'es[20](https://github.com/getsentry/sentry/actions/runs/15033225703/job/42249995049?pr=91686#step:6:21)15' or later.

from: https://github.com/getsentry/sentry/actions/runs/15033225703/job/42249995049?pr=91686

@DanielRosenwasser
Copy link
Contributor Author

I'm seeing the following output, but its obvs set to be "module": "preserve" & "moduleResolution": "bundler" in the PR.

I think this is resolved in Jest 30 (not released yet!):

@scttcper
Copy link
Member

scttcper commented May 16, 2025

@DanielRosenwasser that jest pr was helpful, i think you can force that mode just for ts-node. We only use ts-node for webpack and jest configs and this would kinda backport the change.

  "ts-node": {
    "transpileOnly": true,
    "compilerOptions": {
      "moduleResolution": "Node10"
    }
  }

@github-actions github-actions bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label May 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants