Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

New: Add visitor keys #516

Merged
merged 5 commits into from
Oct 31, 2018
Merged

Conversation

michalsnik
Copy link
Contributor

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: vuejs/vue-cli#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: vuejs/vue-eslint-parser#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
Copy link

jsf-clabot commented Sep 23, 2018

CLA assistant check
All committers have signed the CLA.

@michalsnik michalsnik changed the title Add visitor keys New: Add visitor keys Sep 23, 2018
@JamesHenry
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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

@mysticatea
Copy link
Member

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
Copy link
Member

mysticatea commented Oct 9, 2018

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

@JamesHenry
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor Author

Conflicts resolved

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants