Skip to content

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

Merged
nicolo-ribaudo merged 8 commits intobabel:masterfrom
kaicataldo:babel-eslint-dynamic-import
Dec 11, 2019
Merged

@babel/eslint-parser: fix ImportExpression node to match ESTree spec #10828
nicolo-ribaudo merged 8 commits intobabel:masterfrom
kaicataldo:babel-eslint-dynamic-import

Conversation

@kaicataldo
Copy link
Copy Markdown
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.

Comment thread packages/babel-parser/src/parser/expression.js Outdated
@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
@kaicataldo kaicataldo force-pushed the babel-eslint-dynamic-import branch from e8fbad0 to 2b0b1ee Compare December 6, 2019 17:43
`);
});

it("Dynamic Import", () => {
Copy link
Copy Markdown
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
Copy Markdown
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 kaicataldo force-pushed the babel-eslint-dynamic-import branch from 2bcbc0c to c25b1e7 Compare December 6, 2019 21:25
@kaicataldo
Copy link
Copy Markdown
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 :)

Comment thread packages/babel-parser/test/fixtures/estree/dynamic-import/basic/output.json Outdated
@nicolo-ribaudo
Copy link
Copy Markdown
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)

@kaicataldo kaicataldo force-pushed the babel-eslint-dynamic-import branch from ee14f2d to 93a3451 Compare December 10, 2019 16:16

// ImportExpressions do not have an arguments array.
toReferencedListDeep(
exprList: $ReadOnlyArray<?N.Expression> = [],
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@kaicataldo kaicataldo force-pushed the babel-eslint-dynamic-import branch from 4a76f37 to 2e77d76 Compare December 10, 2019 16:44
@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.

4 participants