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

Fix type annotations range #31

Closed
wants to merge 2 commits into from

Conversation

michalsnik
Copy link
Member

This PR fixes ranges in typeAnnotation nodes.

Full discussion and description of the problem can be found here: vuejs/vue-cli#1672 (comment)

Essentially, because of not updated range of typeAnnotation - the rule space-infix-ops reported wrong nodes causing confusion among typescript users.

@codecov-io
Copy link

codecov-io commented Aug 11, 2018

Codecov Report

Merging #31 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
+ Coverage   85.89%   85.92%   +0.02%     
==========================================
  Files          32       32              
  Lines        1879     1883       +4     
  Branches      486      488       +2     
==========================================
+ Hits         1614     1618       +4     
  Misses        180      180              
  Partials       85       85
Impacted Files Coverage Δ
src/ast/nodes.ts 100% <ø> (ø) ⬆️
src/script/index.ts 84.4% <100%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da877ec...ca85b03. Read the comment docs.

* Top-level type annotation from `typescript-eslint-parser` and `babel-eslint`
*/
export interface TypeAnnotation extends HasLocation, HasParent {
type: "TSTypeAnnotation" | "TypeAnnotation"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know there are many more possible types, but they are rather children of these top level types. And what we need here at this moment is just the top level type annotation on Identifier. Unless there will be introduced another eslint rule that will respect further cases of typeAnnotation or returnType. What do you think?

@mysticatea
Copy link
Member

mysticatea commented Aug 12, 2018

Thank you for this PR. However, I'd like to go another way.

This is a known issue which is described in README.md.

If you use typescript-eslint-parser, the location of original nodes can be wrong. Waiting for typescript-eslint-parser to support parseResult.visitorKeys.
https://github.com/mysticatea/vue-eslint-parser#parseroptionsparser

The root cause is typescript-eslint-parser doesn't support parseResult.visitorKeys. Therefore, vue-eslint-parser couldn't know that the AST that typescript-eslint-parser generated has some custom node types.

I'd like to add the parseResult.visitorKeys support to typescript-eslint-parser in order to fix this problem.

@michalsnik
Copy link
Member Author

Alright, I wasn't aware of the visitorKeys. Would you like to proceed with this? I won't be able to tackle it in the next few weeks unfortunately.

@michalsnik
Copy link
Member Author

michalsnik commented Sep 23, 2018

I added visitorKeys to typescript-eslint-parser here: eslint/typescript-eslint-parser#516

So I'm going to close this PR and wait for the feedback there :) It indeed addresses the issue more thoroughly 👍

@michalsnik michalsnik closed this Sep 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants