Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

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

@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

cc @devsnek

@joyeecheung
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

cc @targos I believe this is semver-minor?

@guybedford
Copy link
Contributor

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

@joyeecheung
Copy link
Member Author

@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

@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

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

@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

@ryzokuken ryzokuken ryzokuken approved these changes

@guybedford guybedford guybedford approved these changes

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