eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

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

Open strmer15 opened this issue 2 years ago β€’ 15 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues and my issue is unique
  • [X] 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 avatar Jul 18 '23 22:07 strmer15

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

ljharb avatar Jul 18 '23 22:07 ljharb

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

JoshuaKGoldberg avatar Jul 18 '23 22:07 JoshuaKGoldberg

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

ljharb avatar Jul 18 '23 22:07 ljharb

eye twitches

JoshuaKGoldberg avatar Jul 18 '23 22:07 JoshuaKGoldberg

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.

Haprog avatar Aug 22 '23 11:08 Haprog

A PR would be great.

ljharb avatar Aug 22 '23 14:08 ljharb

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 (https://github.com/jsx-eslint/eslint-plugin-react/issues/3602#issuecomment-1641078594). 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 πŸ‘‡ πŸ˜„

JoshuaKGoldberg avatar Aug 22 '23 14:08 JoshuaKGoldberg

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

ljharb avatar Aug 22 '23 15:08 ljharb

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. πŸ™‚

Haprog avatar Aug 23 '23 06:08 Haprog

Ah got it, sorry for jumping at you there! πŸ˜„

JoshuaKGoldberg avatar Aug 23 '23 06:08 JoshuaKGoldberg

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

Haprog avatar Aug 23 '23 07:08 Haprog

Any updates on this?

Yuhao-C avatar Apr 09 '24 18:04 Yuhao-C

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

ljharb avatar Apr 09 '24 18:04 ljharb

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

benasher44 avatar Apr 22 '24 19:04 benasher44

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

ljharb avatar Apr 22 '24 21:04 ljharb