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 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
Collaborator

ljharb left a comment

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

@HauptmannEck

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Oct 24, 2018

Sure, that seems reasonable.

@HauptmannEck

This comment has been minimized.

Copy link
Author

HauptmannEck commented Oct 24, 2018

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.

@ljharb
ljharb approved these changes Oct 24, 2018
@ljharb ljharb requested review from yannickcr, EvHaus and lencioni Oct 24, 2018
@EvHaus
EvHaus approved these changes Oct 24, 2018
Copy link
Collaborator

EvHaus left a comment

LGTM

@ljharb ljharb merged commit a92a0fb into yannickcr:master Oct 24, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 97.453%
Details
@HauptmannEck

This comment has been minimized.

Copy link
Author

HauptmannEck commented Oct 29, 2018

@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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Oct 29, 2018

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

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Dec 28, 2018

v7.12.0 is released.

This was referenced Jan 4, 2019
@ibf-renovate ibf-renovate mentioned this pull request Jan 12, 2019
0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.