-
Notifications
You must be signed in to change notification settings - Fork 364
Add experimental representation for dynamic import #137
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
Conversation
Just for reference the babylon pr babel/babylon#163 (I linked it in the webpack issue) Originally was interface Import <: Node {
type: "Import";
} interface CallExpression <: Expression {
type: "CallExpression";
- callee: Expression | Super;
+ callee: Expression | Super | Import;
...
} aside: next time we implement a proposal I'l remember to make an issue here too or ping everyone? |
@hzoo any reason y'all chose a new node? The reason Super had a new node was that it took multiple syntatic forms (Call Expression and Member Expression). Not sure what the argument is for creating a new node type when it seems like this is allowed anywhere an expression is allowed. |
@hzoo would be great to start here if the goal is eventual interop. |
Got it from babel/babylon#163 (comment).
Ah makes sense |
That's fair, I think at the time of the original discussion on Babylon we hadn't even nailed down if it was restricted to a single argument, so a general CallExpression made sense. Now that it's locked down, we could change that up. |
I'm happy to make the corresponding changes to Babylon based on what is decided. |
(stage 2, not 3, btw) |
Not sure how this changes the discussion, but it's highly highly likely that there will be |
https://github.com/rwaldron/tc39-notes/blob/d0c651b358b361b0855cfe7af9ba0b76cab73949/es7/2015-09/sept-24.md has some mentions of "import.context" - I don't think there's concrete plans yet, but I know that any context-sensitive information would need to be reachable through a metaproperty on |
@mikesherov yes, it's similar to |
Ok, I feel convinced that something like @hzoo suggested (but with Although I don't really like how special-case that we had for |
@RReverser metaproperties (like |
@ljharb We do have it for metaproperties. I was speaking about speciality of call itself (that keyword is allowed only in callee position and not anywhere else) - for that matter I liked |
ok @RReverser can you revise PR so we can bikeshed here and then change in appropriate consumers? |
|
@mikesherov Updated. |
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.
LGTM
@@ -0,0 +1,13 @@ | |||
# [Dynamic Import Expression](https://github.com/domenic/proposal-dynamic-import) | |||
|
|||
## ImportExpression |
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.
Should the file name and heading here still be ImportExpression
or do we also want to call it dynamic import?
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.
good catch.
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.
It seems like "Import Expression" better follows existing ESTree constructs. "Dynamic" isn't really a syntactic description IMHO.
Btw, yeah - do we want to call it |
|
Another alternative name may be |
We ought to agree on the node type:
|
@guybedford, if/when import gets metadata properties, wouldn't |
@domenic This isn't part of the grammar, but AST name. We do want it to be like |
Doesn't the same problem apply to super? super call vs. super property access. |
@domenic No, we use same |
Right, so if you don't find that confusing, I don't see why using |
Sounds like the remaining non-controversial node name (as the |
@domenic |
@ariya Except that |
@RReverser If you take |
Yeah, I think we're safe to call it So I'd like then to nail down this proposal: simply one node type |
I'm fine with |
Sounds great. I'll update PR when back on Monday. |
Ok, great! Good news @hzoo, seems like we'll converge on what Babylon already has! |
Slightly relevant: can The proposal only lists ImportCall as an addition to the production grammar of CallExpression. |
It wouldn't make much sense for |
The main confusion I think that can happen with |
111214a
to
7b35d00
Compare
Updated and removed relation between |
@guybedford Developer needs either to look at AST definition or use TypeScript definitions anyway. In that case, it will be crystal clear that |
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.
LGTM
Ok, thanks everyone for your input. I thumbed the PR. Feel free to land @RReverser |
Adding suggested representation for dynamic
import
expression stage 2 proposal: https://github.com/domenic/proposal-dynamic-importThis one is quite simple and for now will be only under
experimental
folder, but if there are any concerns or suggestions, please let me know :)cc @domenic @mikesherov @ariya