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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

@babel/eslint-parser: fix ImportExpression node to match ESTree spec #10828

Merged
merged 8 commits into from Dec 11, 2019

Conversation

kaicataldo
Copy link
Member

Q A
Fixed Issues? Refs #10826
Patch: Bug Fix? 馃憤
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@babel/parser currently parses dynamic imports (import('a')) as CallExpressions (code here). The ESTree spec differs from this and gives it its own type called ImportExpression.

@kaicataldo kaicataldo force-pushed the babel-eslint-dynamic-import branch 2 times, most recently from a9dca47 to e51ed79 Compare December 6, 2019 06:20
@nicolo-ribaudo nicolo-ribaudo added area: estree pkg: parser PR: Bug Fix 馃悰 A type of pull request used for our changelog categories labels Dec 6, 2019
@@ -518,5 +518,11 @@ describe("babylon-to-espree", () => {
}
`);
});

it("Dynamic Import", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Nice to be able to test this across the parser and the estree plugin :)

@kaicataldo
Copy link
Member Author

Wrestling with some Flow types right now (I haven't used Flow in years 馃槵), but will hopefully have that fixed up soon.

@kaicataldo
Copy link
Member Author

Still struggling with the flow types on this one (trying to avoid using any). If someone sees what I'm doing wrong, I'd love your input :)

@nicolo-ribaudo
Copy link
Member

The problem with flow is that a function returns X, you can't override it with a function which returns X | Y because it's not compatible: the caller wouldn't know how to handle Y. (I have fixed it)


// ImportExpressions do not have an arguments array.
toReferencedListDeep(
exprList: $ReadOnlyArray<?N.Expression> = [],
Copy link
Member Author

Choose a reason for hiding this comment

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

This was crashing because it's given the CallExpression's arguments array, which is undefined for ImportExpressions (deleted above).

Copy link
Member

Choose a reason for hiding this comment

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

What about doing this? We avoid using the empty array and iterating it:

toReferencedListDeep(exprList: ?$ReadOnlyArray<?N.Expression>) {
  if (exprList) return super.toReferencedListDeep(exprList);
}

If flow complains that it can't return undefined, we can just change the original toReferencedListDeep signature to return void, since it's return value is never used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that sounds good to me. Updated!

@kaicataldo kaicataldo force-pushed the babel-eslint-dynamic-import branch 2 times, most recently from 575753b to 4a76f37 Compare December 10, 2019 16:43
@nicolo-ribaudo nicolo-ribaudo merged commit 7b54a94 into babel:master Dec 11, 2019
@kaicataldo kaicataldo deleted the babel-eslint-dynamic-import branch December 11, 2019 16:18
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 12, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: estree outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 馃悰 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants