-
-
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, v7, v8 use typeArguments with fallback to typeParameters
#3629
[Fix] @typescript-eslint
v6, v7, v8 use typeArguments with fallback to typeParameters
#3629
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3629 +/- ##
==========================================
- Coverage 97.62% 94.42% -3.21%
==========================================
Files 132 132
Lines 9692 9703 +11
Branches 3520 3522 +2
==========================================
- Hits 9462 9162 -300
- Misses 230 541 +311 ☔ 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. |
…rst-prop-new-line`, `jsx-props-no-multi-spaces`, `propTypes`: use type args
|
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). |
- only install peer dependencies with legacy mode when testing @typescript-eslint/parser < v6
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? |
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 :-) |
380e32c
to
51d342b
Compare
e3ee2b0
to
6509e19
Compare
@ljharb I read in #3602 (comment) that you're open to pulling in additional changes to this PR, rather than opening another PR, so I'm here to submit my branch which updates the workflows to pass the tests. In order to get the workflows to run properly I had to disable legacy peer dependency installation for jobs running v6+ of the parser, since it requires it's peer dependencies (typescript in this case) to be installed correctly. I have also added v7 and v8 to the test matrix (you can check the runs for more details), since that is kind of blocking our Eslint 9 upgrade right now 🙂 #3629 (comment) turned out to be an issue, but only very minor changes required in the code 👍 |
I have now rebased my branch so it's up to date with the latest changes in master 👍 Please let me know if there's more I can help out with to land this @ljharb! 🙂 |
6509e19
to
1535846
Compare
I've pulled in your changes; let's see how the tests fare :-) (update: they passed! i'll give this a thorough review and go from there) |
@typescript-eslint
v6 use typeArguments with fallback to typeParameters@typescript-eslint
v6, v7, v8 use typeArguments with fallback to typeParameters
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.
Overall LGTM!
1535846
to
28a33dc
Compare
Follows the guide from
@typescript-eslint
to upgrade tov6
, by adding a small utility functiongetTypeArguments
which returns thetypeArguments
or falls back totypeParameters
.Closes #3602