New: Add visitor keys - Githubissues

michalsnik commented 5 years ago

This PR implements visitor keys, as described in the official ESLint documentation

Reasoning: There is a problem while using Vue with Typescript (with eslint-plugin-vue) and some of the reported errors point to wrong locations in the file. The original issue can be found here: https://github.com/vuejs/vue-cli/issues/1672

I nailed it down to a problem with vue-eslint-parser that didn't fix locations of typeAnnotations, so I posted the PR there: https://github.com/mysticatea/vue-eslint-parser/pull/31

This however is only a quick fix. In order to fully fix the problem and make sure we properly traverse and fix locations of each AST node produced by typescript-eslint-parser inside vue-eslint-parser we need to have visitorKeys setup in here and this is what I'm exactly doing in this PR.

I went through the whole parser's code and tried to catch everything I could in terms of those keys, but it's possible I didn't catch it all, so I'd be grateful for feedback if there's anything I didn't consider initially.

jsf-clabot commented 5 years ago

All committers have signed the CLA.

JamesHenry commented 5 years ago

Hi @michalsnik, thanks for this really valuable contribution!

I apologise for the inconvenience but this parser just underwent a major internal refactor in which the core parsing logic was extracted and put into its own freestanding project.

This doesn't impact the need nor implementation of your PR, but there are some conflicts. Hopefully they should be relatively straightforward to resolve

JamesHenry commented 5 years ago

I have requested review from @mysticatea as he originally suggested creating this PR and was the one who implemented the relevant custom parser API in ESLint

michalsnik commented 5 years ago

Thank you @JamesHenry, sure thing! I updated my PR so it doesn't conflict with the parser anymore.

mysticatea commented 5 years ago

Hi. I'm sorry for the late response. I had been away from OSS activity because of some health problem.

The implementation looks good to me.

However, I guess that we need more investigation because the visitorKeys API has more impacts:

Scope analysis: it will get to check type nodes. TypeScript has two namespaces, variables and types, so this might trigger confusing to ESLint's variable scopes. (Though probably people has disabled the rules which check variables/references and used tsc.)
Core traversal: every rule will get to traverse type nodes. I think that this is the right way, but I guess some rules to be confused. I'm wondering we should have a plugin to place alternative of some core rules, such as eslint-plugin-babel.

mysticatea commented 5 years ago

I did small downstream check then I see no problem on core rules in my some repositories at least.

JamesHenry commented 5 years ago

@mysticatea Thanks!

Core traversal: every rule will get to traverse type nodes. I think that this is the right way, but I guess some rules to be confused. I'm wondering we should have a plugin to place alternative of some core rules, such as eslint-plugin-babel.

We have https://github.com/nzakas/eslint-plugin-typescript

kaicataldo commented 5 years ago

Looks like there are some merge conflicts now.

@JamesHenry Looks like @mysticatea approved this - how do you feel about merging this once the conflicts are resolved?

michalsnik commented 5 years ago

Conflicts resolved

eslint / typescript-eslint-parser

New: Add visitor keys #516