Skip to content
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

Merged
merged 30 commits into from Jun 30, 2023

Conversation

JoshuaKGoldberg
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg commented Jun 6, 2023

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:

  • ✅ Running with typescript@5.1.0-dev.20230301 does not exhibit the OOM error
  • ❌ Running with typescript@5.1.0-dev.20230302 does exhibit the OOM error

Our 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.

Thanks to @jakebailey from the TypeScript team for helping us with this! ❤️


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.

@typescript-eslint
Copy link
Contributor

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.

@netlify
Copy link

netlify bot commented Jun 6, 2023

Deploy Preview for typescript-eslint failed.

Name Link
🔨 Latest commit 3a01637
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/649f1795a0c6860007244359

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review June 7, 2023 02:15
@JoshuaKGoldberg
Copy link
Member Author

Run ./.github/actions/wait-for-netlify
/home/runner/work/typescript-eslint/typescript-eslint/node_modules/@actions/core/lib/core.js:106
        throw new Error(`Input required and not supplied: ${name}`);
        ^

Error: Input required and not supplied: netlify_token

@JamesHenry something funky with the new action, perhaps? https://github.com/typescript-eslint/typescript-eslint/actions/runs/5195237524/jobs/9367800550?pr=7088

@JoshuaKGoldberg
Copy link
Member Author

PASS  tests/lib/node-utils.test.ts
 PASS  tests/lib/parse.project-true.test.ts
<--- Last few GCs --->
[2972:0x61b15c0]   201164 ms: Mark-sweep (reduce) 2037.5 (2051.0) -> 2036.7 (2052.5) MB, 2662.2 / 0.0 ms  (average mu = 0.171, current mu = 0.014) allocation failure scavenge might not succeed
[2972:0x61b15c0]   203868 ms: Mark-sweep (reduce) 2037.8 (2051.5) -> 2037.3 (2052.8) MB, [26](https://github.com/typescript-eslint/typescript-eslint/actions/runs/5195237524/jobs/9367799309?pr=7088#step:6:27)32.6 / 0.0 ms  (average mu = 0.102, current mu = 0.026) allocation failure scavenge might not succeed
<--- JS stacktrace --->
FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0xa3ad50 node::Abort() [/opt/hostedtoolcache/node/14.21.3/x64/bin/node]

sigh.

bradzacher
bradzacher previously approved these changes Jun 7, 2023
Copy link
Member

@bradzacher bradzacher left a 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!

packages/typescript-estree/src/convert.ts Show resolved Hide resolved
packages/typescript-estree/src/convert.ts Show resolved Hide resolved

// Both of these are equivalent:
const x = <Foo a:b="hello" />;
const y = <Foo a : b="hello" />;
Copy link
Member

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!

@JamesHenry
Copy link
Member

JamesHenry commented Jun 7, 2023

Run ./.github/actions/wait-for-netlify
/home/runner/work/typescript-eslint/typescript-eslint/node_modules/@actions/core/lib/core.js:106
        throw new Error(`Input required and not supplied: ${name}`);
        ^

Error: Input required and not supplied: netlify_token

@JamesHenry something funky with the new action, perhaps? https://github.com/typescript-eslint/typescript-eslint/actions/runs/5195237524/jobs/9367800550?pr=7088

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

@JamesHenry
Copy link
Member

@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

@JamesHenry
Copy link
Member

I thought that should work based on this answer: https://github.com/orgs/community/discussions/26409#discussioncomment-3251822

anyone know the right syntax here?

@bradzacher
Copy link
Member

That's all we've got here:

if: github.repository == 'typescript-eslint/typescript-eslint' && github.ref == 'refs/heads/main'

IDK why it wouldn't be working there?

@bradzacher
Copy link
Member

@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.

@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented Jun 9, 2023

Ah, this may be from a different TypeScript issue likely to be fixed soon: microsoft/TypeScript#54542

@JoshuaKGoldberg JoshuaKGoldberg removed the blocked by another issue Issues which are not ready because another issue needs to be resolved first label Jun 26, 2023
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #7088 (3a01637) into main (f74862c) will decrease coverage by 0.04%.
The diff coverage is 33.33%.

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     
Flag Coverage Δ
unittest 87.39% <33.33%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/typescript-estree/src/convert.ts 29.22% <0.00%> (-0.15%) ⬇️
...ipt-estree/src/parseSettings/warnAboutTSVersion.ts 93.33% <100.00%> (ø)

... and 2 files with indirect coverage changes

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"
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Collaborator

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.

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.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 4bf2d73 into typescript-eslint:main Jun 30, 2023
36 of 43 checks passed
@JoshuaKGoldberg JoshuaKGoldberg deleted the typescript-5.1 branch June 30, 2023 23:21
// @ts-check

const ts = require('typescript');
console.log('Running with TypeScript version:', ts.version);
Copy link
Member Author

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 😄

aimichelle pushed a commit to pixie-io/pixie that referenced this pull request Jul 13, 2023
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>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript 5.1 Support
5 participants