-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: update TypeScript to 5.0 RC #6570
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
feat: update TypeScript to 5.0 RC #6570
Conversation
Thanks for the PR, @sosukesuzuki! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
packages/typescript-estree/src/create-program/createIsolatedProgram.ts
Outdated
Show resolved
Hide resolved
@@ -21,6 +21,7 @@ describe('convert', () => { | |||
function fakeUnknownKind(node: ts.Node): void { | |||
ts.forEachChild(node, fakeUnknownKind); | |||
// @ts-expect-error -- intentionally writing to a readonly field | |||
// eslint-disable-next-line deprecation/deprecation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use other node for testing?
packages/typescript-estree/src/create-program/getWatchProgramsForProjects.ts
Show resolved
Hide resolved
c9be94f
to
d53c67c
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6570 +/- ##
==========================================
- Coverage 90.66% 88.62% -2.04%
==========================================
Files 376 382 +6
Lines 12851 12883 +32
Branches 3783 3787 +4
==========================================
- Hits 11651 11418 -233
- Misses 856 1124 +268
+ Partials 344 341 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
d53c67c
to
49e64cb
Compare
@@ -113,6 +113,7 @@ export const LoadedEditor: React.FC<LoadedEditorProps> = ({ | |||
jsx, | |||
parseTSConfig(tsconfig).compilerOptions, | |||
); | |||
// @ts-expect-error Monaco typescript.CompilerOptions is incompatible with typescript 5.0 types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this when Monaco updates CompilerOptions
type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiousity, what's incompatible here? I'm trying to get monaco up to date among other things, so I'm curious.
Monaco has to redeclare everything for their public API, so, it's likely that this happens a lot and casing this away forever might be better (unless the problem is trivial).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without @ts-expect-error
, TS output the error Type 'ModuleResolutionKind.Classic' is not assignable to type 'ModuleResolutionKind | undefined'.
packages/typescript-estree/src/create-program/getWatchProgramsForProjects.ts
Show resolved
Hide resolved
@@ -107,10 +107,10 @@ | |||
"ts-node": "10.7.0", | |||
"tslint": "^6.1.3", | |||
"tsx": "^3.12.1", | |||
"typescript": ">=3.3.1 <5.0.0" | |||
"typescript": ">=3.3.1 <4.9.5 || 5.0.1-rc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to be so strict here? If we release 4.9.6 (however unlikely), that'll cause problems. That and I see basically no future in which we change the APIs post-RC, so it feels pretty safe to just bump this to < 5.1.0
.
Though, I also think that you shouldn't specify versions here at all; given ts-eslint prints a very loud warning about using a potentially incompatible TypeScript version, this particular bit IMO only causes pain when trying to get package managers happy in the TypeScript repo or when testing newer releases of TypeScript in other repos...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the private package.json for our repo, and is not published to the outside world.
Each package that actually depends on TS uses an optional, unbounded peer dependency on TS:
typescript-eslint/packages/eslint-plugin/package.json
Lines 78 to 82 in a249412
"peerDependenciesMeta": { | |
"typescript": { | |
"optional": true | |
} | |
}, |
This range here is primarily here as our "top level" range to clearly specify the range for the project. At some point we want to automatically flow this value into our docs and package as the single point-of-definition.
It actually doesn't even do anything technically because just below it we have a resolution which forces a specific version of TS (eg this PR sets it to ~5.0.1-rc
).
I agree though that we should probably keep it as <5.0.0
instead of dropping it down to <4.9.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, oops. I'm conflating this with the fact that prereleases in the tree don't satisfy "*" or "latest" and cause duplication (boo).
if (typeof type.value === 'object') { | ||
// Print ts.PseudoBigInt | ||
return `${ | ||
type.value.negative ? '-' : '' | ||
}${type.value.base10Value.toString()}n`; | ||
} | ||
return type.value.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled instead by the condition on line 111
We should move that branch up instaed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looking good to me!
I think we can merge this once we have the syntax changes ready
@sosukesuzuki In my project, I've upgraded to
Is this a bug or do I simply not having something configured correctly? |
Semver matching of preleases basically doesn't happen; to use a prerelease you would need to add an That being said, TS 5.0 was just released, so the best answer will be to get ts-eslint's version change to allow the real deal. |
PR Checklist
Overview
Updates
typescript
to5.0.1-rc
! Please see commits for details.Todo
eslint-plugin