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

[Bug]: @typescript-eslint v6 throws deprecation warnings #3602

Closed
2 tasks done
strmer15 opened this issue Jul 18, 2023 · 15 comments · Fixed by #3629
Closed
2 tasks done

[Bug]: @typescript-eslint v6 throws deprecation warnings #3602

strmer15 opened this issue Jul 18, 2023 · 15 comments · Fixed by #3629

Comments

@strmer15
Copy link

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • My issue appears in the command-line and not only in the text editor

Description Overview

Using @typescript-eslint/parser 6.x throws a warning on the command line saying DeprecationWarning: The 'typeParameters' property is deprecated on CallExpression nodes. Use 'typeArguments' instead.

My project is an ejected CRA config; this prints out on the command line every time that I run eslint src on my project after I upgraded my @typescript-eslint dependencies to use the latest v6.

See https://typescript-eslint.io/linting/troubleshooting/#the-key-property-is-deprecated-on-type-nodes-use-key-instead-warnings for more info.

Expected Behavior

A warning should not be printed for deprecations.

eslint-plugin-react version

v7.32.2

eslint version

v8.45.0

node version

v18.16.1

@strmer15 strmer15 added the bug label Jul 18, 2023
@ljharb
Copy link
Member

ljharb commented Jul 18, 2023

cc @bradzacher @JoshuaKGoldberg is this by chance an easy PR?

@JoshuaKGoldberg
Copy link

Yup, it is! Replace node.typeParameters with (node.typeArguments ?? node.typeParameters). That'll support both v5 and v6 of typescript-eslint.

@ljharb
Copy link
Member

ljharb commented Jul 18, 2023

i assume we can use || as well, since we don't have a build process here :-)

@JoshuaKGoldberg
Copy link

eye twitches

@Haprog
Copy link

Haprog commented Aug 22, 2023

You may not be able to simply use || for this in case there are valid logic paths where typeParameters is falsy since then it would trigger the deprecation warning. There are some places (at least in propTypes.js) where there is logic for checking if typeParameters exists before using it so I assume this is the case.

I think doing this instead should work:

const typeArgs = 'typeArguments' in node ? node.typeArguments : node.typeParameters;

and then use typeArgs instead of node.typeParameters.

Though it seems like similar handling needs to be done for usage of node.parent.typeArguments and node.superTypeArguments in some places.

I was testing this a bit and was able to get rid of this deprecation warning by monkey patching this kind of changes in node_modules directly.
If I find some time I might contribute a PR that fixes this if you think this approach is ok.

@ljharb
Copy link
Member

ljharb commented Aug 22, 2023

A PR would be great.

@JoshuaKGoldberg
Copy link

JoshuaKGoldberg commented Aug 22, 2023

monkey patching this kind of changes

I'm just a maintainer on typescript-eslint, not eslint-plugin-react, but: please don't monkey patch modules if it's not absolutely necessary. You never know when those patches break downstream tools or are themselves broken by package updates. Especially when there's a recommended workaround in the thread (#3602 (comment)). I can't guarantee that we won't frequently break the patch by refactors on our end.

@Haprog if the node.typeArguments || node.typeParameters switch is presenting issues, I'd love to see specifics. We can always update https://typescript-eslint.io/blog/announcing-typescript-eslint-v6 and/or the deprecation notice with tailored instructions.

Edit: what ljharb said 👇 😄

@ljharb
Copy link
Member

ljharb commented Aug 22, 2023

It’s fine to do such patching temporarily to test out a PR you want to make, to be clear, just don’t commit or deploy it or anything :-P

@Haprog
Copy link

Haprog commented Aug 23, 2023

Yes, of course. What I meant with monkey patching in this case was that I was just locally modifying eslint-plugin-react package in my project's node_modules to see what it would take to fix the issue to verify the feasibility of the suggested fix before making a PR to eslint-plugin-react. So patching just for the sake of testing the idea. 🙂

@JoshuaKGoldberg
Copy link

Ah got it, sorry for jumping at you there! 😄

@Haprog
Copy link

Haprog commented Aug 23, 2023

@JoshuaKGoldberg btw related to the instruction here https://typescript-eslint.io/linting/troubleshooting/#the-key-property-is-deprecated-on-type-nodes-use-key-instead-warnings

If you've using many ESLint plugins, have updated each to their latest version, and you're not sure which one this complaint is coming from, try disabling half of them at a time to narrow down which plugin it is.

This was not very helpful even though I was only using about 5 plugins since disabling a plugin you also need to remove or comment out the rules related to that plugin and also need to remove related "eslint-disable " comments in your code to be able to run without errors (otherwise you'll get errors about missing rule definitions).

I found a much easier way to figure out which plugin is causing issues is to run with Node's --trace-deprecation option. For example since I was seeing these deprecation warnings when running npm test I could instead run with

npx cross-env NODE_OPTIONS=--trace-deprecation npm test

to get better debugging output and see which plugin/file/line is causing the issues without disabling any plugins/rules.

This will give output like so:

(node:82846) DeprecationWarning: The 'typeParameters' property is deprecated on TSTypeReference nodes. Use 'typeArguments' instead. See https://typescript-eslint.io/linting/troubleshooting#the-key-property-is-deprecated-on-type-nodes-use-key-instead-warnings.
    at Object.defineProperty.get (/Users/<redacted>/node_modules/@typescript-eslint/typescript-estree/dist/convert.js:2477:29)
    at DeclarePropTypesForTSTypeAnnotation.searchDeclarationByName (/Users/<redacted>/node_modules/eslint-plugin-react/lib/util/propTypes.js:630:53)
    at DeclarePropTypesForTSTypeAnnotation.visitTSNode (/Users/<redacted>/node_modules/eslint-plugin-react/lib/util/propTypes.js:597:14)
    at Array.forEach (<anonymous>)
    at DeclarePropTypesForTSTypeAnnotation.convertIntersectionTypeToPropTypes (/Users/<redacted>/node_modules/eslint-plugin-react/lib/util/propTypes.js:723:20)
    at DeclarePropTypesForTSTypeAnnotation.visitTSNode (/Users/<redacted>/node_modules/eslint-plugin-react/lib/util/propTypes.js:606:14)
    at DeclarePropTypesForTSTypeAnnotation.visitTSNode (/Users/<redacted>/node_modules/eslint-plugin-react/lib/util/propTypes.js:595:14)
    at new DeclarePropTypesForTSTypeAnnotation (/Users/<redacted>/node_modules/eslint-plugin-react/lib/util/propTypes.js:583:12)
    at markPropTypesAsDeclared (/Users/<redacted>/node_modules/eslint-plugin-react/lib/util/propTypes.js:975:34)
    at markAnnotatedFunctionArgumentsAsDeclared (/Users/<redacted>/node_modules/eslint-plugin-react/lib/util/propTypes.js:1056:9)

@Yuhao-C
Copy link

Yuhao-C commented Apr 9, 2024

Any updates on this?

@ljharb
Copy link
Member

ljharb commented Apr 9, 2024

nope, or they'd be posted on the issue.

@benasher44
Copy link

Are maintainers open to PRs to fix it? I can make some time in the next week or so.

@ljharb
Copy link
Member

ljharb commented Apr 22, 2024

@benasher44 yes! altho #3629 already exists, so instead of a new PR, please comment on that PR with a link to a branch or commit, and I'll pull in your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants