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

Use loc version of end, to handle parsers that don't fill name.end. #2026

Merged
merged 2 commits into from Oct 24, 2018

Conversation

HauptmannEck
Copy link

@HauptmannEck HauptmannEck commented Oct 24, 2018

Not all parsers fill the name.end field in the AST structure so relying on it is less robust.
Ref: babel/babel#6681
But the range syntax is widely supported (babel has a flag).

I ran into this issue using the typescript-eslint-parser which does not add start and end to the nodes, but has range.

Here is an example to see the difference:
https://astexplorer.net/#/gist/c336be172990d7b467d1ed3dbc1882ee/d312c83cd24626d2e82ba791add48fe061b7cdc0

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This sounds good, but can we add test cases that would have failed without this change?

@HauptmannEck
Copy link
Author

HauptmannEck commented Oct 24, 2018

@ljharb It should be possible, but I would need to add typescript-eslint-parser to the devDependencies to test that route. Sadly babel-eslint does not have an option for the start and end to be disabled.

If it is ok to add a new package to devDependencies then I should be able to create a test

@ljharb
Copy link
Member

ljharb commented Oct 24, 2018

Sure, that seems reasonable.

@HauptmannEck
Copy link
Author

It ended up needed typescript as well to run the parser, I don't know if there is a better/lighter parser for showing this issue.

Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

LGTM

@ljharb ljharb merged commit a92a0fb into jsx-eslint:master Oct 24, 2018
@HauptmannEck
Copy link
Author

@ljharb Quick question, what is the release policy for this repo? Is there a certain timeline for releases or does it just deped on a accumulation of changes? Just curious, as I am not blocked by this fix.

@ljharb
Copy link
Member

ljharb commented Oct 29, 2018

There’s no policy and no set timeline; things get released when maintainers have time to do so.

@ljharb
Copy link
Member

ljharb commented Dec 28, 2018

v7.12.0 is released.

This was referenced Jan 4, 2019
@ghost ghost mentioned this pull request Jan 12, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants