Update dynamic imports AST #844
Conversation
|
@marijnh estree/estree#198 has been merged, so this PR is ready. Would you review this PR? |
acorn/src/expression.js
Outdated
| this.next() // skip `(` | ||
|
|
||
| // Parse node.source. | ||
| if (this.type === tt.ellipsis) { |
There was a problem hiding this comment.
Why are the explicit checks (the ellipsis and also the comma below) for things that would raise an error anyway? I'd prefer the terse variant that just allows the callee methods to raise the errors.
There was a problem hiding this comment.
This is the same behavior as before this PR. I will remove it.
acorn/src/expression.js
Outdated
| this.raise(node.callee.start, "Cannot use new with import(...)") | ||
| } | ||
| if (this.eat(tt.parenL)) node.arguments = this.parseExprList(tt.parenR, this.options.ecmaVersion >= 8 && node.callee.type !== "Import", false) | ||
| node.callee = this.parseSubscripts(this.parseExprAtom(null, true), startPos, startLoc, true) |
There was a problem hiding this comment.
What is the advantage of adding a new argument to parseExprAtom over the way the check worked before?
There was a problem hiding this comment.
Because
new (import(s))is the valid syntax.new import.metais the valid syntax, in the future. If acorn throws the parse error there, we makeimport.metaplugin relatively hard.
There was a problem hiding this comment.
Hm. I will replace the new argument by another way.
|
I have updated this PR. |
| // Parse node.source. | ||
| node.source = this.parseMaybeAssign() | ||
|
|
||
| // Verify ending. |
There was a problem hiding this comment.
Thanks for adjusting the ellipses thing. Any reason the 8 lines below can't simply be this.expect(tt.parenR)?
There was a problem hiding this comment.
I wanted to show a detailed message for trailing commas because import(s) is similar to call expressions, but doesn't support trailing commas. I.e., it handles import(s,) as special, but not import(a,b) and import(s!.
There was a problem hiding this comment.
I have moved the logic to the errored code path.
|
👍 |
|
Great, merged! |
|
Thanks a lot for the fast action! 🐎 Any chance of a new release with this change? |
|
I've tagged 7.0.0 with this AST format change and |
This PR changes the AST of dynamic imports as following estree/estree#198. Please don't merge this PR til estree/estree#198 was merged. (estree/estree#198 is waiting for @adrianheine's response.)
This PR adds the second parameter to
parser.parseExprAtom()method to make the following behavior:new import(s)is a syntax error.new (import(s))is not a syntax error. (though it's a runtime type error.)new import.meta(s)is not a syntax error in the future. (though it will be a runtime type error.)Also, this PR adds some test cases:
import(a = 'dynamicImport.js')new (import(s))import((s,t))(import)(s)