chore: switch to production typescript#612
typescript#612Conversation
With TypeScript 7 out, we no longer need the `@typescript/native-preview` package as 7.x is the prod version of that.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe workspace replaces the native TypeScript preview compiler with TypeScript 7, adds a catalog alias and dependency, and updates the typecheck script to invoke TypeScript 7 directly. ChangesTypeScript toolchain
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
|
It seems that ESLint is not working well with TypeScript v7 yet |
|
@antfu TSESLint isn't. but that should be fine.. this is what nested dependencies are for. im not sure why TSESLint is falling over here. it should have a dependency (or peer dependency) of ill have to dig a bit more but TSESLint is at fault, not us |
|
TSESLint requires TS API, even if type-aware linting is not enabled. |
|
yep fully understood 👍 that's not the problem here. the problem here is that pnpm should be giving TSESLint the TS it asks for ( this PR was created already knowing TSESLint doesn't support TS 7.x yet. it should still be possible by normal, nested dependency resolution. |
pnpm doesn't install peers by version, so we end up with one copy of typescript even though TSESLint wants another one. This obviously causes TSESLint to error at runtime. The only solution seems to be to alias it for now until the day TSESLint supports 7.x.
Conflicts messed it up.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pnpm-workspace.yaml (1)
20-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the stale native-preview catalog entry.
@typescript/native-previewis still declared here, but static analysis reports that nopackage.jsonuses it. This currently fails the unit-test/lint check and contradicts the intended removal.- '`@typescript/native-preview`': 7.0.0-dev.20260707.2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpm-workspace.yaml` at line 20, Remove the stale '`@typescript/native-preview`' entry from the workspace catalog in pnpm-workspace.yaml, leaving all other catalog dependencies unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pnpm-workspace.yaml`:
- Line 20: Remove the stale '`@typescript/native-preview`' entry from the
workspace catalog in pnpm-workspace.yaml, leaving all other catalog dependencies
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a861f3ef-04f3-493e-9073-525f00390968
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.jsonpnpm-workspace.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
| "simple-git-hooks": "catalog:dev", | ||
| "tsdown": "catalog:dev", | ||
| "typescript": "catalog:dev", | ||
| "typescript-7": "catalog:dev", |
There was a problem hiding this comment.
IMHO, we should keep it as @typescript/native-preview, since typescript-7 is not a widely recognized name among tools such as tsdown, nx, and Next.js. As shown in https://grep.app/search?q=@typescript/native-preview, they are still using the existing name. Using a different name (such as typescript-7 or typescript7) within this repository would be fine, but I think we should follow the established convention.
There was a problem hiding this comment.
it doesn't feel any better pretending one package is another. typescript is not @typescript/native-preview. aliasing it as such is flaky and relying on the hope that the two expose the same shape etc.
luckily neither has an API really so it will probably work, but it doesn't make it any better.
there is no existing convention for what we're doing here - aliasing typescript in a way that satisfies pnpm enough to not break TSESLint. you chose to alias it to @typescript/native-preview, but that isn't a convention you followed, it is just your preference.
the only convention i know of for aliasing typescript installs is that of DT:
https://github.com/microsoft/DefinitelyTyped-tools/blob/8b9e12cd2b5680c748838204995618eeb3e3b58a/packages/typescript-packages/package.json#L25-L29
There was a problem hiding this comment.
on a side note, i don't want to bikeshed here. if you feel like we can't move on without naming this the way you want it named, let's just name it that.
i'm more interested in doing the migration than what we call it
There was a problem hiding this comment.
Since @antfu is also involved in this PR, I'll wait for his input.
|
I personally would prefer to wait a bit for the ecosystem to catch up, as in this repo we are just really a customer of TypeScript; upgrading or not doesn't really change anything other than local development (while unplugin is not a heavy project to gain the benefit for native TS a lot either). When ts7 is widely avaiable, we will start to bump for all the projects anyway. |
With TypeScript 7 out, we no longer need the
@typescript/native-previewpackage as 7.x is the prod version of that.Summary by CodeRabbit