Skip to content

Add initial support for dynamic module import #3204

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

Closed

Conversation

ChadKillingsworth
Copy link
Collaborator

@ChadKillingsworth ChadKillingsworth commented Jan 21, 2019

Adds basic parsing support and code generation for the dynamic import call. See https://tc39.github.io/proposal-dynamic-import/

The specification severely limits usage of import(specifier) type calls. Aliasing, .call, .apply, .bind and many other JS function semantics are not supported.

At the moment, the Refastor tests are failing, but I assume that is because certain passes are being skipped now. I'll need some help in figuring out how to work around that.

First step for full support of #2770

See https://tc39.github.io/proposal-dynamic-import/
Supports parsing and code generation for `import('foo')` type statements.
@lauraharker
Copy link
Contributor

Internal Google issue http://b/123244371 created

@lauraharker lauraharker added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Jan 22, 2019
@brad4d
Copy link
Contributor

brad4d commented Jan 24, 2019

@ChadKillingsworth
I've been looking through the PR and thinking about what we need here.

I believe the code you're proposing is modeling import(module) as a CALL node with an IMPORT node as the first child (callee) and the single module name argument as its second child.

Although it looks like a CALL, it is different in many significant ways that I think will be likely to trip up existing compiler code.

Could you change this implementation to instead define a new TokenType for Node, possibly DYNAMIC_IMPORT or IMPORT_CALL? This will ensure that existing code doesn't trip on it and will avoid some of the special casing you had to add in your current version to deal with an IMPORT that has no children.

Thanks.

@ChadKillingsworth
Copy link
Collaborator Author

@brad4d Switched to using a new node type DYNAMIC_IMPORT with a single argument.

Also verified in Chrome that only a single argument is expected (not an argument list).
import('foo', 'bar') doesn't parse.

@brad4d
Copy link
Contributor

brad4d commented Jan 28, 2019

imported for internal testing and review

@brad4d
Copy link
Contributor

brad4d commented Feb 5, 2019

found the source of at least some of the test failures.
We need to put the new feature in ES_UNSUPPORTED (which didn't exist when this PR was created) instead of ES_NEXT.

I've done that in my internal version.
I don't plan to push my changes back to this PR's branch unless I feel I need more input from Chad, though.

@brad4d
Copy link
Contributor

brad4d commented Feb 8, 2019

had another small internal breakage, which I've fixed today
sent to an additional internal reviewer to get approval for that fix

@brad4d
Copy link
Contributor

brad4d commented Feb 11, 2019

I'm submitting this change internally now.
If nothing fails, then it will appear in GitHub tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes internal-issue-created An internal Google issue has been created to track this GitHub issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants