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

Update: support bigint and dynamic import #415

Merged
merged 6 commits into from
Aug 14, 2019

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Jul 13, 2019

This PR adds supports of bigint and dynamic import.

  • It enables bigint and dynamic import if ecmaVersion option was 2020 or 11.
  • It represents bigint literal as Literal node that has bigint property. The bigint property has the source code text of the bigint literal without the suffix n.
  • It represents dynamic import as ImportExpression node. The source property is the importing source as similar to ImportDeclaration node, but the source property can be an arbitrary expression node.

@@ -29,6 +29,11 @@ function getRaw(ast) {
return undefined; // eslint-disable-line no-undefined
}

// JSON cannot handle BigInt.
if (typeof value === "bigint") {
return null;
Copy link
Member Author

Choose a reason for hiding this comment

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

The tests depend on JSON heavily, but JSON doesn't support BigInt values. So tests handle Literal#value as null. I think that this is not a big problem because it's null if runtime doesn't support bigint natively.

mysticatea and others added 3 commits July 14, 2019 07:39
Looks like the fixture previously assumed the `expression` node would be where the error is located, but it looks like the error is located at the `import` token instead.
@platinumazure
Copy link
Member

@mysticatea I've tried to push a few changes to consume the new acorn and eslint-visitor-keys, and then fixed a failing test. Please review the latest commits on this branch and let me know if there are issues. (Thinking about it now, I should have made a PR targeting this branch so you could review using the PR review tools. Sorry about that.)

@mysticatea
Copy link
Member Author

@platinumazure Thank you so much!

@mysticatea
Copy link
Member Author

LGTM.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

The changes LGTM, but I would like another team member to review this since I have also pushed changes to this branch. Thanks!

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 💯


script:
- if [ $TRAVIS_NODE_VERSION -ge 8 ]; then node Makefile.js lint; fi
- node Makefile.js test
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity: this was added since eslint v6 don't support node <8?

we may consider dropping node <8, as it has been EOL.

Copy link
Member

Choose a reason for hiding this comment

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

@aladdin-add I assume that was the case (but hopefully @mysticatea can confirm). I think it makes sense to release a minor version of espree with this functionality if we can, and then we could do a major release to drop Node <8.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for working on this!

@platinumazure platinumazure merged commit f5e58cc into master Aug 14, 2019
@platinumazure platinumazure deleted the support-bigint-and-dynamic-import branch August 14, 2019 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants