Skip to content

Conversation

RReverser
Copy link
Member

@RReverser RReverser commented Oct 25, 2016

Adding suggested representation for dynamic import expression stage 2 proposal: https://github.com/domenic/proposal-dynamic-import

This 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

@hzoo
Copy link
Contributor

hzoo commented Oct 25, 2016

Just for reference the babylon pr babel/babylon#163 (I linked it in the webpack issue)

Originally was ImportCallExpression similar to ImportExpression but was changed to be a new Import node similar to Super https://github.com/estree/estree/blob/master/es2015.md#expressions

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?

@mikesherov
Copy link
Contributor

@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.

@mikesherov
Copy link
Contributor

@hzoo would be great to start here if the goal is eventual interop.

@hzoo
Copy link
Contributor

hzoo commented Oct 25, 2016

Got it from babel/babylon#163 (comment).

The reason Super had a new node was that it took multiple syntatic forms (Call Expression and Member Expression)

Ah makes sense

cc @loganfsmyth @kesne @ljharb

@loganfsmyth
Copy link
Contributor

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.

@kesne
Copy link

kesne commented Oct 25, 2016

I'm happy to make the corresponding changes to Babylon based on what is decided.

@ljharb
Copy link

ljharb commented Oct 25, 2016

(stage 2, not 3, btw)

@ljharb
Copy link

ljharb commented Oct 25, 2016

Not sure how this changes the discussion, but it's highly highly likely that there will be import.foo forms in the future - for example, to syntactically provide information about the current module (like __dirname does now in CJS).

@mikesherov
Copy link
Contributor

@ljharb that does change it alot, because then it is more like super. Any public signals about that form you can point to @ljharb ?

@ljharb
Copy link

ljharb commented Oct 25, 2016

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 import.

@jhnns
Copy link

jhnns commented Oct 25, 2016

@mikesherov yes, it's similar to super(), see tc39/proposal-dynamic-import#8 (comment)

@RReverser
Copy link
Member Author

RReverser commented Oct 25, 2016

Ok, I feel convinced that something like @hzoo suggested (but with source field) would be a better way.

Although I don't really like how special-case that we had for Super now becomes more common in new proposals instead of sticking with common expression syntax rules, but that discussion is outside of the ESTree scope anyway.

@ljharb
Copy link

ljharb commented Oct 25, 2016

@RReverser metaproperties (like super.foo, new.target, function.sent, etc) are going to become increasingly common, so there needs to be some pattern to express them.

@RReverser
Copy link
Member Author

RReverser commented Oct 25, 2016

@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 System.import slightly more.

@mikesherov
Copy link
Contributor

ok @RReverser can you revise PR so we can bikeshed here and then change in appropriate consumers?

@ljharb
Copy link

ljharb commented Oct 26, 2016

import() is better than System.import precisely because it's syntactic, so it can be a special form that can have special knowledge about usage context.

@RReverser
Copy link
Member Author

@mikesherov Updated.

mikesherov
mikesherov previously approved these changes Oct 26, 2016
Copy link
Contributor

@mikesherov mikesherov left a 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
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch.

Copy link
Contributor

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.

@mikesherov mikesherov dismissed their stale review October 26, 2016 15:48

file name is incorrect

@RReverser
Copy link
Member Author

Btw, yeah - do we want to call it Import or DynamicImport? First one is shorter (like Super) but might be easily confused with import declarations or specifiers.

@guybedford
Copy link

Import does seem a little too generic to me. I think DynamicImport would be preferable as clarity should outweigh brevity. Although backwards compatibility will likely be an issue though at this stage.

@guybedford
Copy link

Another alternative name may be ContextualImport to cater to possible metadata properties in future.

@ariya
Copy link
Contributor

ariya commented Jan 27, 2017

We ought to agree on the node type:

  • Import: already in Babel, little too generic, possible confusion with import declaration/specifier
  • DynamicImport: not really a syntactic description
  • ContextualImport
  • ImportExpression: aligned closely with the existing NewExpression and CallExpression
  • ImportCallExpression: minor variant of ImportExpression
  • ImportCall: the name in the grammar production in the proposal

@mikesherov
Copy link
Contributor

@guybedford, if/when import gets metadata properties, wouldn't import in that case by a metaExpression? Meaning I think we only need to worry about a bare import call here, is that right?

@RReverser
Copy link
Member Author

@domenic This isn't part of the grammar, but AST name. We do want it to be like super, but while Super name for AST node is quite clear, Import, as mentioned above, is little too generic and might be confused with ImportDeclaration etc., so would be nice to have more precise name.

@domenic
Copy link

domenic commented Jan 27, 2017

Doesn't the same problem apply to super? super call vs. super property access.

@RReverser
Copy link
Member Author

@domenic No, we use same Super node type in both cases.

@domenic
Copy link

domenic commented Jan 27, 2017

Right, so if you don't find that confusing, I don't see why using Import here would be confusing.

@ariya
Copy link
Contributor

ariya commented Jan 27, 2017

Sounds like the remaining non-controversial node name (as the callee) is ImportCall.

@RReverser
Copy link
Member Author

@domenic Import is confusing not because of context, but rather conflicts with existing ImportSpecifier, ImportDeclaration, ImportDefaultSpecifier and ImportNamespaceSpecifier - it might sound like something that is common for all of them, not as a separate syntactic element unrelated to these.

@RReverser
Copy link
Member Author

@ariya Except that ImportCall would look weird if/when we get metaproperties. Unless you want to name it ImportCallee or something.

@kesne
Copy link

kesne commented Jan 27, 2017

@RReverser If you take Import standalone, then sure it could be confusing, but I don't see how it would be confusing in the AST context of being the callee in a CallExpression.

@mikesherov
Copy link
Contributor

Yeah, I think we're safe to call it Import, and I'm not so sure who would be confused by this!

So I'd like then to nail down this proposal: simply one node type Import, and differentiate based on whether it's parent node is a CallExpression or a MetaExpression.

@ariya
Copy link
Contributor

ariya commented Jan 28, 2017

I'm fine with Import as the node name.

@RReverser
Copy link
Member Author

Sounds great. I'll update PR when back on Monday.

@mikesherov
Copy link
Contributor

Ok, great! Good news @hzoo, seems like we'll converge on what Babylon already has!

@ariya
Copy link
Contributor

ariya commented Jan 29, 2017

Slightly relevant: can Import be the callee of NewExpression as well and not only CallExpression?

The proposal only lists ImportCall as an addition to the production grammar of CallExpression.

@Jessidhia
Copy link

Jessidhia commented Jan 30, 2017

It wouldn't make much sense for Import to be the callee with new. if it were allowed, it'd either be the same footgun as new require('foo') (calls require as a constructor) or it'd construct a (promise for a) module record, which is not callable or constructible.

@guybedford
Copy link

The main confusion I think that can happen with Import is that for a new developer looking at a transform using a visitor pattern, it may not be obvious that Import is visiting a dynamic import, and not a static import. But this confusion isn't really a concern of this spec I guess, and Import does seem to make complete semantic sense.

@RReverser RReverser force-pushed the RReverser-import-expr branch from 111214a to 7b35d00 Compare January 30, 2017 09:36
@RReverser
Copy link
Member Author

Updated and removed relation between CallExpression and NewExpression - Indeed, with introduction of special callees it doesn't make sense any more.

@RReverser
Copy link
Member Author

@guybedford Developer needs either to look at AST definition or use TypeScript definitions anyway. In that case, it will be crystal clear that Import accepts Expression while regular ImportDeclaration etc. only accept static strings.

Copy link
Contributor

@mikesherov mikesherov left a comment

Choose a reason for hiding this comment

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

LGTM

@mikesherov
Copy link
Contributor

Ok, thanks everyone for your input. I thumbed the PR. Feel free to land @RReverser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.