-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: support TypeScript 5.1 #7088
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: support TypeScript 5.1 #7088
Conversation
Thanks for the PR, @JoshuaKGoldberg! 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 failed.
|
c975543
to
16d750f
Compare
@JamesHenry something funky with the new action, perhaps? https://github.com/typescript-eslint/typescript-eslint/actions/runs/5195237524/jobs/9367800550?pr=7088 |
sigh. |
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.
One case that would be worth adding a test for:
<foo-bar:baz-bam />
Both the ns and name of the namespace are allowed to be valid JSX identifiers!
Otherwise this is all LGTM
Once you land it we can do an out-of-band release to get it out and keep the early adopters happy!
|
||
// Both of these are equivalent: | ||
const x = <Foo a:b="hello" />; | ||
const y = <Foo a : b="hello" />; |
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.
cool - this case actually used to be an error pre 5.1!
Oh sorry, I reckon it could be that the secret doesn’t work as an input when you’re coming from a fork… I will figure out how this has been workable for other actions in the past |
@JoshuaKGoldberg @bradzacher I can't find any evidence that this ever worked. I think the secret won't be populated on forks which makes sense. I have therefore just pushed a commit to the branch which should should hopefully amend things to only run the Website Tests when not on a fork |
I thought that should work based on this answer: https://github.com/orgs/community/discussions/26409#discussioncomment-3251822 anyone know the right syntax here? |
That's all we've got here: typescript-eslint/.github/workflows/ci.yml Line 248 in a2b6b2e
IDK why it wouldn't be working there? |
@JoshuaKGoldberg once you've got all the tests passing locally feel free to just land this to main. We can follow up with a fix commit if any CIs fail on main. |
This reverts commit c6f949b.
Ah, this may be from a different TypeScript issue likely to be fixed soon: microsoft/TypeScript#54542 |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7088 +/- ##
==========================================
- Coverage 87.43% 87.39% -0.04%
==========================================
Files 386 386
Lines 13192 13198 +6
Branches 3872 3873 +1
==========================================
+ Hits 11534 11535 +1
- Misses 1292 1296 +4
- Partials 366 367 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
package.json
Outdated
@@ -112,10 +111,10 @@ | |||
"ts-node": "10.7.0", | |||
"tslint": "^6.1.3", | |||
"tsx": "^3.12.1", | |||
"typescript": ">=3.3.1 <5.1.0" | |||
"typescript": "npm:@typescript-deploys/pr-build@5.2.0-pr-54781-9" |
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.
@JoshuaKGoldberg is the regression going to be patch fixed in 5.1? Or is it going to wait for 5.2?
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.
@jakebailey indicated it should be patched soon
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.
Yeah there will be a patch release of 5.1 in the next day or so.
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.
5.1.6 is out on npm since yesterday, although not visible in https://github.com/microsoft/TypeScript/releases yet.
// @ts-check | ||
|
||
const ts = require('typescript'); | ||
console.log('Running with TypeScript version:', ts.version); |
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.
oops, meant to remove this... will do later 😄
Summary: To get the fixes from [this saga of bugs](typescript-eslint/typescript-eslint#7088). Relevant Issues: N/A Type of change: /kind bugfix Test Plan: `yarn typecheck && yarn lint`. Editors should still behave. Signed-off-by: Nick Lanam <nlanam@pixielabs.ai>
Note: This doesn't Block You From Linting TypeScript 5.1
You can still use
typescript@>=5.1
with@typescript-eslint/eslint-plugin
&@typescript-eslint/parser
. There are no breaking changes in TS 5.1 that would block you from linting existing code. JSX namespaces (<MyComponent a:b />
) should generally be linted properly.Until this PR is merged and a new set of typescript-eslint package versions are released, your console may see a log like
WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.
.Status Update: June 26th 2023
Looks like TypeScript's latest releases have fixed the issue. We should be ready to release a new version with this PR's changes soon - pending review.
Status Update: June 9 2023
This PR is blocked on out-of-memory (OOM) errors running a set of important unit tests for the parser. The OOM error only occurs with TypeScript 5.1, not locally.
We're working on identifying which commit to TypeScript introduced the change that coincides with this OOM error being introduced. In #7091 we narrowed it down as follows:
typescript@5.1.0-dev.20230301
does not exhibit the OOM errortypescript@5.1.0-dev.20230302
does exhibit the OOM errorOur next step will be to write a script to run against individual TypeScript commits.It looks like microsoft/TypeScript#54542 is the root of the OOM. At least, from running on recent TypeScript commits, that's the first one that has the OOM occur. TBD once it's fixed.
PR Checklist
Overview
Bumps the supported ranges per https://typescript-eslint.io/maintenance/versioning/dependant-version-upgrades#adding-support-for-a-new-typescript-version. Since we didn't have an RC PR, this merges the PR steps a bit.