[dynamic-import] Implementing import() syntax #163

kesne · 2016-10-08T22:55:01Z

There's a new stage-2 proposal that adds a new special import() function. This implements the syntax behind a importFunctions plugin.

Here's the proposal: https://github.com/domenic/proposal-import-function

cc: @domenic

domenic · 2016-10-08T23:00:51Z

Thanks so much for working on this; this'll be very helpful.

As a nit, I think it might be better to name it importFunctionLike so people don't get the wrong idea. It's more like super() than it is like a function. I'll probably rename the proposal repo similarly (tc39/proposal-dynamic-import#14).

kesne · 2016-10-08T23:06:45Z

Yeah I thought about that working on this, that's probably not a bad idea. I'm also not 💯 on ImportCallExpression in the AST.

codecov-io · 2016-10-08T23:13:20Z

Current coverage is 94.46% (diff: 90.00%)

No coverage report found for master at 2697bfd.

Powered by Codecov. Last update 2697bfd...e203d34

loganfsmyth · 2016-10-08T23:57:39Z

src/parser/expression.js

@@ -300,7 +300,8 @@ pp.parseSubscripts = function (base, startPos, startLoc, noCalls) {
      let node = this.startNodeAt(startPos, startLoc);
      node.callee = base;
      node.arguments = this.parseCallExpressionArguments(tt.parenR, possibleAsync);
-      base = this.finishNode(node, "CallExpression");
+      base = this.finishNode(node, this.state.inImportCall ? "ImportCallExpression" : "CallExpression");


Perhaps we'd be better off having this be its own node type, like super? super() for instance is just a normal CallExpression with callee being a Super node. Should we introduce an Import node as well?

That's probably a good idea, I can rework this to do that.

@loganfsmyth Alright I updated the code to do that. Ended up being a pretty clean solution! Thanks for the recommendation.

loganfsmyth · 2016-10-09T00:27:35Z

src/parser/expression.js

@@ -394,6 +404,7 @@ pp.parseExprAtom = function (refShorthandDefaultPos) {
      if (this.state.inGenerator) this.unexpected();

    case tt.name:
+      if (!this.hasPlugin("importFunctions") && this.state.type === tt._import) this.unexpected();


Looks like this isn't needed now?

Yep, good catch! Fixed.

loganfsmyth · 2016-10-09T00:34:41Z

src/parser/statement.js

+          break;
+        } else {
+          this.state = state;
+        }


I think this can be simplified to this, though I'm not an expert on Babylon.

if (this.hasPlugin("importFunctions") && this.lookahead().type === tt.parenL) break;

Yep, looks like that handles this use-case perfectly.

ljharb · 2016-10-09T01:19:01Z

Is it a good idea to ensure in the tests that it parses in both sloppy and strict mode, and in Scripts and Modules?

kesne · 2016-10-09T01:22:27Z

@ljharb Not a bad idea. I can also add a test case for calling in module mode (right now I just run in script mode and assume that if it passes in script, it passes in module).

EDIT: Added both of those cases!

kesne · 2016-10-09T01:58:34Z

src/parser/expression.js

+
+      node = this.startNode();
+      this.next();
+      if (!this.match(tt.parenL)) {


It looks like this case can never be hit, because the parser runs through statements first, and would throw unexpected trying to parse the import function.

Should I remove it knowing that, or keep it for safety?

I wondered that too, good question, curious to see what others think.

Also codecoverage says we never hit this block, but as we do the same for super maybe be consistent and leave it here?

I figured out that this can be hit if you do something that parses as an expression first. For example, inside of a function return import.fails() hits this case.

I've added a test for this case to prevent any regressions and to bump the code coverage.

danez · 2016-10-10T08:56:08Z

src/parser/expression.js

+
+      node = this.startNode();
+      this.next();
+      if (!this.match(tt.parenL)) {


Also codecoverage says we never hit this block, but as we do the same for super maybe be consistent and leave it here?

kesne · 2016-10-10T16:58:01Z

@domenic Considering this: tc39/proposal-dynamic-import#15 do you think that we should enforce the single argument syntax in the parser now?

domenic · 2016-10-10T17:14:03Z

Yeah, that's probably a good idea... not sure what the spec grammar will look like yet, which might impact your AST...

hzoo · 2016-10-10T18:32:08Z

src/parser/expression.js

@@ -385,6 +385,16 @@ pp.parseExprAtom = function (refShorthandDefaultPos) {
      }
      return this.finishNode(node, "Super");

+    case tt._import:
+      if (!this.hasPlugin("importFunctions")) this.unexpected();


Doesn't have to be done in this PR since it's a separate issue but we should make a better error message here (whenever we have a this.hasPlugin()

I agree that would be nice. I did this because it seems like this is how the rest of the plugins handle it.

Yeah it's totally fine just a note so I don't forget - I can just make an actual issue

kesne · 2016-10-10T18:47:07Z

Alright, I added a check to the arguments which fails parse on more than one argument. The only remaining line that I don't have code coverage for is the check for the plugin in expressions. If you folks think that'd be good to have I can do that.

domenic · 2016-10-10T22:18:34Z

Updates from the spec side:

The new alphanumeric name of the import() proposal is "proposal-dynamic-import"
The spec now requires exactly one argument. (Might be worth testing the no-argument import() case too!)

Exciting!

kesne · 2016-10-11T02:38:20Z

I renamed the plugin dynamicImport, and updated it to require exactly one argument and added a test to check for the no-arg case!

kesne · 2016-10-14T18:06:22Z

Is there anything else that I can do to help get this out?

hzoo · 2016-10-14T18:20:21Z

Kind of trivial but maybe a test that import() fails to parse without the plugin turned on?

we can do it later

hzoo · 2016-10-14T18:32:00Z

src/parser/expression.js

+      node = this.startNode();
+      this.next();
+      if (!this.match(tt.parenL)) {
+        this.unexpected();


Maybe we can do a

this.unexpected(null, `Unexpected token, expected (`);

https://github.com/babel/babylon/pull/150/files#diff-51a873a9697282bed0c15722a6ccf4e6R77

not sure if you can just use this.expect instead

Actually we can use #172

to do this.unexpected(null, tt.parenL);

Should we add this to #172 after this lands?

Done in #177.

[import-functions] First pass at import()

1dc5228

kesne changed the title ~~[import-functions] First pass at import()~~ [import-functions] Implementing import() syntax Oct 8, 2016

Fixing lint error

d5df72d

kesne mentioned this pull request Oct 8, 2016

[import()] Initial support for dynamic-import babel/babel#4699

Merged

loganfsmyth reviewed Oct 8, 2016

View reviewed changes

Changing import-functions to use Import callee

d700775

loganfsmyth reviewed Oct 9, 2016

View reviewed changes

Removing unused code in expression switch

69a1060

loganfsmyth reviewed Oct 9, 2016

View reviewed changes

Jordan Gensler added 2 commits October 8, 2016 17:37

Simplifying import function break in statement

afa4da6

Adding new Import callee to docs

33fc379

[import()] Adding tests for strict mode and module parsing

0eee521

kesne commented Oct 9, 2016

View reviewed changes

danez approved these changes Oct 10, 2016

View reviewed changes

danez added Tag: New Feature spec-update labels Oct 10, 2016

Jordan Gensler added 2 commits October 10, 2016 09:53

[import()] Adding test for expression parsing

d330f8f

[spec] Adding back missing super docs

2a6dab6

hzoo reviewed Oct 10, 2016

View reviewed changes

[import()] Throwing on more than one argument

0fb758f

[import()] Updating for renamed proposal

e203d34

kesne changed the title ~~[import-functions] Implementing import() syntax~~ [dynamic-import] Implementing import() syntax Oct 11, 2016

hzoo mentioned this pull request Oct 12, 2016

Better error message on unexpected token when it's because a plugin isn't turned on #169

Closed

hzoo reviewed Oct 14, 2016

View reviewed changes

hzoo merged commit c63c1bc into babel:master Oct 14, 2016

This was referenced Oct 14, 2016

Fix up #172 #177

Merged

A plugin for nested imports. (T7408) babel/babel#4329

Closed

This was referenced Oct 20, 2016

import() support in webpack 2 webpack/webpack#3098

Closed

Add experimental representation for dynamic import estree/estree#137

Merged

kristofdegrave pushed a commit to kristofdegrave/babylon that referenced this pull request Oct 27, 2016

[dynamic-import] Implementing import() syntax (babel#163)

f10ce61

hzoo mentioned this pull request Dec 5, 2016

Add babel-generator support for Import babel/babel#4945

Merged

islemaster mentioned this pull request Oct 11, 2019

Part 4: Lazy load sound picker and image picker code-dot-org/code-dot-org#30937

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dynamic-import] Implementing import() syntax #163

[dynamic-import] Implementing import() syntax #163

kesne commented Oct 8, 2016

domenic commented Oct 8, 2016 •

edited

Loading

kesne commented Oct 8, 2016

codecov-io commented Oct 8, 2016 •

edited

Loading

loganfsmyth Oct 8, 2016

kesne Oct 9, 2016

kesne Oct 9, 2016

loganfsmyth Oct 9, 2016

kesne Oct 9, 2016

loganfsmyth Oct 9, 2016

kesne Oct 9, 2016

ljharb commented Oct 9, 2016 •

edited

Loading

kesne commented Oct 9, 2016 •

edited

Loading

kesne Oct 9, 2016

loganfsmyth Oct 9, 2016

danez Oct 10, 2016 •

edited

Loading

kesne Oct 10, 2016

danez Oct 10, 2016 •

edited

Loading

kesne commented Oct 10, 2016

domenic commented Oct 10, 2016

hzoo Oct 10, 2016 •

edited

Loading

kesne Oct 10, 2016

hzoo Oct 10, 2016

kesne commented Oct 10, 2016

domenic commented Oct 10, 2016

kesne commented Oct 11, 2016 •

edited

Loading

kesne commented Oct 14, 2016

hzoo commented Oct 14, 2016 •

edited

Loading

hzoo Oct 14, 2016

hzoo Oct 14, 2016

kesne Oct 14, 2016

hzoo Oct 14, 2016

motiz88 Oct 14, 2016

[dynamic-import] Implementing import() syntax #163

[dynamic-import] Implementing import() syntax #163

Conversation

kesne commented Oct 8, 2016

domenic commented Oct 8, 2016 • edited Loading

kesne commented Oct 8, 2016

codecov-io commented Oct 8, 2016 • edited Loading

Current coverage is 94.46% (diff: 90.00%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Oct 9, 2016 • edited Loading

kesne commented Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danez Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danez Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

kesne commented Oct 10, 2016

domenic commented Oct 10, 2016

hzoo Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kesne commented Oct 10, 2016

domenic commented Oct 10, 2016

kesne commented Oct 11, 2016 • edited Loading

kesne commented Oct 14, 2016

hzoo commented Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Oct 8, 2016 •

edited

Loading

codecov-io commented Oct 8, 2016 •

edited

Loading

ljharb commented Oct 9, 2016 •

edited

Loading

kesne commented Oct 9, 2016 •

edited

Loading

danez Oct 10, 2016 •

edited

Loading

danez Oct 10, 2016 •

edited

Loading

hzoo Oct 10, 2016 •

edited

Loading

kesne commented Oct 11, 2016 •

edited

Loading

hzoo commented Oct 14, 2016 •

edited

Loading