Skip to content

[10.x] vm: add dynamic import support#25421

Closed
joyeecheung wants to merge 1 commit intonodejs:v10.x-stagingfrom
joyeecheung:backport-22381-to-10
Closed

[10.x] vm: add dynamic import support#25421
joyeecheung wants to merge 1 commit intonodejs:v10.x-stagingfrom
joyeecheung:backport-22381-to-10

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 9, 2019

PR-URL: #22381
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Guy Bedford guybedford@gmail.com
Reviewed-By: Tiancheng "Timothy" Gu timothygu99@gmail.com

@devsnek @joyeecheung
vm: add dynamic import support
3060cdf 
PR-URL: nodejs#22381
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 9, 2019

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2210/pipeline

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v10.x labels Jan 9, 2019
@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 9, 2019

cc @devsnek

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 9, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20033/

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 9, 2019

cc @targos I believe this is semver-minor?

@guybedford
Copy link
Contributor

guybedford commented Jan 10, 2019

I would suggest holding off on this while #25424 remains a concern.

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 10, 2019

@guybedford I am fine with that, but be aware this unblocks clean cherry-pick of other refactoring patches to the bootstrap process

@guybedford
Copy link
Contributor

guybedford commented Jan 10, 2019

@joyeecheung I understand. As it is an experimental API we could go ahead with the merge despite the bug if it would help follow-on PR work, we just need to continue to note that the importModuleDynamically API on vm.Script should be considered experimental, with known bugs, and that it may well change.

@guybedford
Copy link
Contributor

guybedford commented Jan 10, 2019

Personally I think we should change the API to tie the dynamic import callback to the context.

@jdalton jdalton mentioned this pull request Jan 10, 2019
Closed
3 tasks
@guybedford
Copy link
Contributor

guybedford commented Jan 26, 2019

@joyeecheung assuming #21573 lands now, we should probably stick with backporting this to continue to enable backports despite the experimental woes mentioned.

@codebytere codebytere force-pushed the v10.x-staging branch 3 times, most recently from 2d6e145 to 7840f71 Compare January 29, 2019 18:12
@BethGriggs
Copy link
Member

BethGriggs commented Feb 5, 2019 •
edited
Loading

Landed on v10.x-staging in 134d1e9

@BethGriggs BethGriggs closed this Feb 5, 2019
BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
@devsnek @BethGriggs
vm: add dynamic import support
134d1e9 
Backport-PR-URL: #25421
PR-URL: #22381
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@devsnek @rvagg
vm: add dynamic import support
0d1500d 
Backport-PR-URL: #25421
PR-URL: #22381
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guybedford guybedford guybedford approved these changes

+1 more reviewer

@ryzokuken ryzokuken ryzokuken approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung @nodejs-github-bot @guybedford @BethGriggs @ryzokuken @devsnek

Comments