-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[Fix] @typescript-eslint
v6 use typeArguments with fallback to typeParameters
#3629
base: master
Are you sure you want to change the base?
[Fix] @typescript-eslint
v6 use typeArguments with fallback to typeParameters
#3629
Conversation
…rst-prop-new-line`, `jsx-props-no-multi-spaces`, `propTypes`: use type args
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3629 +/- ##
==========================================
- Coverage 97.71% 97.58% -0.13%
==========================================
Files 133 133
Lines 9480 9491 +11
Branches 3475 3477 +2
==========================================
- Hits 9263 9262 -1
- Misses 217 229 +12 ☔ View full report in Codecov by Sentry. |
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.
Seems good, altho there's an uncovered line.
Hopefully this is fixed and merged soon. |
@HenryBrown0 any update on those smaller PRs? |
Sorry I got side tracked, I'll try getting some tests written in the next few days |
0f45d97
to
586b162
Compare
Can we add some tests that would have failed without these typeArguments checks? That may require adding tests that run on typescript-eslint v6. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@typescript-eslint/parser@5.62.0 |
Fair, I'll take that (and rebase this) assuming tests pass :-) thanks! |
looking forward to this! |
2929087
to
f5bd822
Compare
@HenryBrown0 unfortunately a number of tests are failing. any idea why? |
Unit tests are failing on master for me, has a dependency changed? It appears only minor version of Node.js 6 are failing, e.g. 18.6.8 and 19.6.8. I suspect this is a red herring as the matrix build reports using different node versions;
|
Thanks, in that case I'll check out master and report back. |
@HenryBrown0 tests seem to be passing on master https://github.com/jsx-eslint/eslint-plugin-react/actions (i checked on my fork before pushing to this repo, so i expect these to pass). |
@@ -37,7 +37,7 @@ const parsers = { | |||
BABEL_ESLINT: path.join(__dirname, NODE_MODULES, 'babel-eslint'), | |||
'@BABEL_ESLINT': path.join(__dirname, NODE_MODULES, '@babel/eslint-parser'), | |||
TYPESCRIPT_ESLINT: path.join(__dirname, NODE_MODULES, 'typescript-eslint-parser'), | |||
'@TYPESCRIPT_ESLINT': path.join(__dirname, NODE_MODULES, '@typescript-eslint/parser'), | |||
'@TYPESCRIPT_ESLINT': path.join(__dirname, NODE_MODULES, '@typescript-eslint/parser/dist'), |
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.
@typescript-eslint/parser
removed the main
field in v6
, which pointed to dist/index.js
. Not sure how tests were passing before, as this would have been wrong for all of v6.
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.
exports['.']
would still work in node versions that support exports, which might be the only node versions that v6 supports?
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.
v6 supports node versions ^16.0.0 || >=18.0.0
, but even if I'm running this locally with node 18 it still fails to find the export
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.
interesting, indeed node 18 should work - line 15 in https://unpkg.com/browse/@typescript-eslint/parser@6.19.0/package.json points there.
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.
It works* locally when I switch it to importing the specific file:
'@TYPESCRIPT_ESLINT': path.join(__dirname, NODE_MODULES, '@typescript-eslint/parser/dist'), | |
'@TYPESCRIPT_ESLINT': path.join(__dirname, NODE_MODULES, '@typescript-eslint/parser/dist/index.js'), |
*as in, other things break later 🙃 - https://github.com/jsx-eslint/eslint-plugin-react/pull/3629/files#r1580983575
👋 @HenryBrown0 do you still have time to work on this? We're at v7 in typescript-eslint (https://typescript-eslint.io/blog/announcing-typescript-eslint-v7) and working on v8 - which will remove the old properties altogether. Even if there are still some cases that log for deprecated property access (https://github.com/jsx-eslint/eslint-plugin-react/pull/3629/files#r1511579466) I personally would very much like to see this PR merged. That way the |
I'm going to look at this again this evening. #3629 (comment) was giving me issues before, maybe you have some ideas how this can be resolved? |
package.json
Outdated
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.
Once https://github.com/jsx-eslint/eslint-plugin-react/pull/3629/files#r1454015312 is fixed, we get this lovely crash on any rule that uses @typescript-eslint/parser
:
TypeError: import_typescript.default.isTokenKind is not a function
at forEachToken (/Users/josh/repos/eslint-plugin-react/node_modules/ts-api-utils/lib/index.cjs:260:35)
at Object.forEachComment (/Users/josh/repos/eslint-plugin-react/node_modules/ts-api-utils/lib/index.cjs:309:10)
at convertComments (/Users/josh/repos/eslint-plugin-react/node_modules/@typescript-eslint/typescript-estree/dist/convert-comments.js:40:13)
at astConverter (/Users/josh/repos/eslint-plugin-react/node_modules/@typescript-eslint/typescript-estree/dist/ast-converter.js:56:66)
at parseAndGenerateServices (/Users/josh/repos/eslint-plugin-react/node_modules/@typescript-eslint/typescript-estree/dist/parser.js:170:66)
at Object.parseForESLint (/Users/josh/repos/eslint-plugin-react/node_modules/@typescript-eslint/parser/dist/parser.js:93:80)
at Object.parseForESLint (/Users/josh/repos/eslint-plugin-react/node_modules/eslint/lib/rule-tester/rule-tester.js:314:36)
at parse (/Users/josh/repos/eslint-plugin-react/node_modules/eslint/lib/linter/linter.js:834:22)
at Linter._verifyWithoutProcessors (/Users/josh/repos/eslint-plugin-react/node_modules/eslint/lib/linter/linter.js:1312:33)
at Linter.verify (/Users/josh/repos/eslint-plugin-react/node_modules/eslint/lib/linter/linter.js:1454:57)
at runRuleForItem (/Users/josh/repos/eslint-plugin-react/node_modules/eslint/lib/rule-tester/rule-tester.js:833:35)
...
This is because TypeScript is on an old version that doesn't support the default export:
eslint-plugin-react/package.json
Line 77 in 4467db5
"typescript": "^3.9.9", |
I tried updating the devDependency to ^4.0.2
and then the error becamse:
TypeError: ts.getModifierFlags is not a function
at Object.hasStaticModifierFlag (/Users/josh/repos/eslint-plugin-react/node_modules/typescript-estree/lib/node-utils.js:403:21)
at convert (/Users/josh/repos/eslint-plugin-react/node_modules/typescript-estree/lib/convert.js:996:29)
at convertChild (/Users/josh/repos/eslint-plugin-react/node_modules/typescript-estree/lib/convert.js:77:12)
...
...from the deprecated typescript-estree
package. Which, along with typescript-eslint-parser
, don't support the latest TypeScript versions. So they can't be used in environments that have typescript@4
(roughly - I haven't checked the exact range support).
eslint-plugin-react/package.json
Line 78 in 2e6b557
"typescript-eslint-parser": "^20.1.1" |
When I set const tsOld = false;
in parsers.js
, then all tests pass with typescript@4
and @typescript-eslint/*@6
.
@ljharb I can vaguely guess the right solution is to change around when the typescript-eslint-parser
tests should be run, but I'm not clear on what the exact change should be. Is this something you can do?
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.
I'll take a look - basically, we do some hijinks in our CI to install the right version combinations of things so it'll work.
ae092ef
to
358dfb8
Compare
@HenryBrown0 I found something very similar and wanted to get your thoughts I tested this PR with my codebase and it in fact fixed the original error However, I am now getting a very similar error where |
This is a question for @ljharb, if it's a deprecated warning I imagine the preference would be to get this in first and a pr after for |
Definitely a bit of scope creep for sure. I just figured I'd mention especially since its coming from the same |
It'd be fine to include it in this PR, or a follow-on; but since this one still has lots of failing tests we probably shouldn't increase complexity yet :-) |
Follows the guide from
@typescript-eslint
to upgrade tov6
, by adding a small utility functiongetTypeArguments
which returns thetypeArguments
or falls back totypeParameters
.Closes #3602