This repository has been archived by the owner. It is now read-only.

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

Merged
merged 11 commits into from Oct 14, 2016

Conversation

Projects
None yet
8 participants
@kesne
Copy link
Contributor

kesne commented Oct 8, 2016

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

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

Jordan Gensler
@domenic

This comment has been minimized.

Copy link

domenic commented Oct 8, 2016

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

This comment has been minimized.

Copy link
Contributor Author

kesne commented Oct 8, 2016

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

This comment has been minimized.

Copy link

codecov-io commented Oct 8, 2016

Current coverage is 94.46% (diff: 90.00%)

No coverage report found for master at 2697bfd.

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

@@ -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");

This comment has been minimized.

@loganfsmyth

loganfsmyth Oct 8, 2016

Member

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?

This comment has been minimized.

@kesne

kesne Oct 9, 2016

Author Contributor

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

This comment has been minimized.

@kesne

kesne Oct 9, 2016

Author Contributor

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

@@ -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();

This comment has been minimized.

@loganfsmyth

loganfsmyth Oct 9, 2016

Member

Looks like this isn't needed now?

This comment has been minimized.

@kesne

kesne Oct 9, 2016

Author Contributor

Yep, good catch! Fixed.

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

This comment has been minimized.

@loganfsmyth

loganfsmyth Oct 9, 2016

Member

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;

This comment has been minimized.

@kesne

kesne Oct 9, 2016

Author Contributor

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

Jordan Gensler added some commits Oct 9, 2016

@ljharb

This comment has been minimized.

Copy link

ljharb commented Oct 9, 2016

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

This comment has been minimized.

Copy link
Contributor Author

kesne commented Oct 9, 2016

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


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

This comment has been minimized.

@kesne

kesne Oct 9, 2016

Author Contributor

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?

This comment has been minimized.

@loganfsmyth

loganfsmyth Oct 9, 2016

Member

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

This comment has been minimized.

@danez

danez Oct 10, 2016

Member

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

This comment has been minimized.

@kesne

kesne Oct 10, 2016

Author Contributor

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

danez approved these changes Oct 10, 2016


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

This comment has been minimized.

@danez

danez Oct 10, 2016

Member

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

Jordan Gensler added some commits Oct 10, 2016

@kesne

This comment has been minimized.

Copy link
Contributor Author

kesne commented Oct 10, 2016

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

@domenic

This comment has been minimized.

Copy link

domenic commented Oct 10, 2016

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

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

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

This comment has been minimized.

@hzoo

hzoo Oct 10, 2016

Member

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()

This comment has been minimized.

@kesne

kesne Oct 10, 2016

Author Contributor

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

This comment has been minimized.

@hzoo

hzoo Oct 10, 2016

Member

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

@kesne

This comment has been minimized.

Copy link
Contributor Author

kesne commented Oct 10, 2016

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

This comment has been minimized.

Copy link

domenic commented Oct 10, 2016

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

This comment has been minimized.

Copy link
Contributor Author

kesne commented Oct 11, 2016

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 kesne changed the title [import-functions] Implementing import() syntax [dynamic-import] Implementing import() syntax Oct 11, 2016

@kesne

This comment has been minimized.

Copy link
Contributor Author

kesne commented Oct 14, 2016

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

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 14, 2016

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

we can do it later

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

This comment has been minimized.

@hzoo

hzoo Oct 14, 2016

Member

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

This comment has been minimized.

@hzoo

hzoo Oct 14, 2016

Member

Actually we can use #172

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

This comment has been minimized.

@kesne

kesne Oct 14, 2016

Author Contributor

Should we add this to #172 after this lands?

This comment has been minimized.

@hzoo

hzoo Oct 14, 2016

Member

yeah

This comment has been minimized.

@motiz88

motiz88 Oct 14, 2016

Contributor

Done in #177.

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

1 of 3 checks passed

codecov/patch 90.00% of diff hit (target 94.46%)
Details
codecov/project 94.46% (-0.02%) compared to 7dd45f7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.