search

eslint / typescript-eslint-parser

An ESLint custom parser which leverages TypeScript ESTree to allow for ESLint to lint TypeScript source code.
Other
915 stars 75 forks source link

Fix: Allow to visit typeParameters in VariableDeclarator #581

Closed armano2 closed 5 years ago

armano2 commented 5 years ago

This PR adds typeParameters to VariableDeclarator

typeParameters can be only present if kind=type

armano2 commented 5 years ago

@platinumazure can i have estimate when this change can be released? its blocking me from fixing some of rules.

platinumazure commented 5 years ago

@armano2 Very sorry I missed your comment.

Is this PR still needed given the changes in #589 (i.e., is that PR separate/independent from this one)? Thanks!

armano2 commented 5 years ago

this PR is needed only for estree v6, but its not needed for v7 because there was change that 

type Foo = number

is no longer variable and now its own node TSTypeAliasDeclaration (kind=type got removed) https://github.com/eslint/typescript-eslint-parser/pull/589/commits/6aad31d5edf06d25677651446d9a88fdd29ef990#diff-32f897025e0e51a215a578a256455ab5

platinumazure commented 5 years ago

Thanks, makes sense.

In the PRs for upgrading typescript-estree to 7.x or higher, do we need to go back to visitor-keys.js and remove the override for variable declarations, given the decision to switch the node type for type declarations, if this PR is merged for typescript-estree@5.x?

armano2 commented 5 years ago

yes we will have to do that.

this property is no longer in AST, and it will be better if it's going to be removed when we upgrade to 7.x

platinumazure commented 5 years ago

@armano2 Understood. Sorry, I wasn't clear: If I merge this PR now, do you need to update #584 and #589 to ensure the property is removed from visitor-keys?

If so, then, given this extra work needed on your side: Should I merge this PR at all (and cut a patch release)? Or should we just close this and have people migrate to a new major version of this plugin (one which supports typescript-estree@7.x or typescript-estree@8.x)?

I'm leaning towards just closing this, but if you think there's a strong need to fix this for people stuck on earlier versions of TS, I could merge this and do a release, and then request changes on #584 and #589. Let me know what you think the best way forward is here.

armano2 commented 5 years ago

@platinumazure yes i will have to update them and i don't mind rebasing / removing it there

armano2 commented 5 years ago

any progress on this?

JamesHenry commented 5 years ago

@armano2 We can close this in favour of #596, right? The node kind in question is no longer available