-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Parse import reflection #14926
Parse import reflection #14926
Conversation
0fdd80c
to
4f9b91e
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53242/ |
4f9b91e
to
f6ba14e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this PR preserve parsing of import module from "x"
?
Good catch. I thought |
612d89c
to
a81f8bc
Compare
(fyi @lucacasonato @guybedford) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this new plugin interact with import assertions?
Can we throw if both module
and asserts
as used in the same declaration, while we wait for the two proposals to decide how to integrate?
...l-parser/test/fixtures/experimental/import-reflection/invalid-flow-type-import-2/output.json
Outdated
Show resolved
Hide resolved
@@ -855,6 855,7 @@ export interface ImportDeclaration extends BaseNode { | |||
source: StringLiteral; | |||
assertions?: Array<ImportAttribute> | null; | |||
importKind?: "type" | "typeof" | "value" | null; | |||
module?: boolean | null; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.ImportDeclaration
will assign null
to module
because it is an optional property.
acc7cb4
to
3f9f6c8
Compare
@@ -179,11 179,15 @@ export function ExportDefaultDeclaration( | |||
export function ImportDeclaration(this: Printer, node: t.ImportDeclaration) { | |||
this.word("import"); | |||
this.space(); | |||
this.printInnerComments(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use indent=false
here?
if (nextNextTokenFirstChar === charCodes.lowercaseF) { | ||
// import module from from ... | ||
isImportReflection = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for import module from foo
, just to verify that it errors even if foo
starts with f
?
// import module { x } ... | ||
// This is invalid, we will continue parsing and throw | ||
// a recoverable error later | ||
isImportReflection = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a test for import module "foo"
?
ping |
Co-authored-by: Nicolò Ribaudo <[email protected]>
bdf52e8
to
9467184
Compare
It is expected but not ideal, because an import declaration has more than one whitespace that can not be attached to any adjacent AST nodes: import /* 1 */ { x } /* 2 */ from "foo" assert /* 3 */ { type: "json" };
import /* 1 */ type x from "foo"; In this example, all comments are inner comments of the import declaration. But the generator does not know where it should print inner comments. Currently what we can do is to infer such position from the AST structure (whether the specifier is an ImportSpecifier, whether it contains import assertions, etc). To precisely regenerate sources from AST, we will need structures more refined than |
I mean the comment positions are being swapped, which is kind of weird. |
@@ -0,0 1,3 @@ | |||
{ | |||
"throws": "Unexpected token, expected \"{\" (1:14)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error isn't the best, since {
is not allowed here, but we can iterate on it.
This PR also introduces the new syntax plugin and adds it to babel-standalone. The AST shape is still preliminary, for discussions about the AST shape, please visit estree/estree#287.
I will update the PR should the AST change.