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

Revert "Revert "vm: add importModuleDynamically option to compileFunction"" #35431

Merged
merged 1 commit into from
Feb 5, 2021
Merged

Revert "Revert "vm: add importModuleDynamically option to compileFunction"" #35431

merged 1 commit into from
Feb 5, 2021

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Sep 30, 2020 •
edited
Loading

This reverts commit 2d5d773.

See: #32985
See: #33364
See: #33166
Fixes: #31860

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. vm Issues and PRs related to the vm subsystem. labels Sep 30, 2020
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@devsnek devsnek added request-ci Add this label to start a Jenkins CI on a PR. and removed tools Issues and PRs related to the tools directory. labels Sep 30, 2020
@mcollina
Copy link
Member

@SimenB can you please check this does not make things worse for you?

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@devsnek
Copy link
Member Author

devsnek commented Sep 30, 2020

also cc @nodejs/build as this involves that weird MSVC bug

@SimenB
Copy link
Member

SimenB commented Sep 30, 2020

Exciting!

@mcollina I was never able to reproduce the original issue (beyond seeing it on CI), so I don't think I'll be able to verify anything here. Sorry 🙁 If there's any way of sticking a build of this branch on CI I'm happy to do that to see if the issue pops up?

@mcollina
Copy link
Member

cc @targos

@mcollina
Copy link
Member

@targos @BethGriggs could you please generate a new build like the ones you used to generate back in #33166?

I've tried building this patch myself but I can't reproduce.

@targos
Copy link
Member

targos commented Oct 24, 2020

Build: https://ci-release.nodejs.org/job/iojs+release/6417/

@targos
Copy link
Member

targos commented Oct 24, 2020

https://nodejs.org/download/test/v15.0.0-test2020102412aff38044/

mcollina
mcollina approved these changes Oct 24, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I cannot reproduce this anymore. I would be very careful in backporting this to v14 as there is something there that caused the problem.

@mcollina
Copy link
Member

@devsnek could you rebase?

@SimenB
Copy link
Member

SimenB commented Oct 24, 2020

Note that if you're testing with [email protected] or newer, it no longer uses compileFunction - we moved back to vm.Script due to #35375. Verify jest-runtime is on an older release than that to test with compileFunction

@mcollina
Copy link
Member

I've been using the same setup I used last time (which the last time I used my windows laptop).

aduh95
aduh95 reviewed Feb 5, 2021
View reviewed changes
doc/api/vm.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 5, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@devsnek
vm: add importModuleDynamically option to compileFunction
bf2f2b7 
This reverts commit 2d5d773.

See: nodejs#32985
See: nodejs#33364
See: nodejs#33166
Fixes: nodejs#31860

PR-URL: nodejs#35431
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
@devsnek devsnek merged commit bf2f2b7 into nodejs:master Feb 5, 2021
@devsnek devsnek deleted the revert-revert-ugh branch February 5, 2021 15:19
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@devsnek @danielleadams
vm: add importModuleDynamically option to compileFunction
66a14d3 
This reverts commit 2d5d773.

See: #32985
See: #33364
See: #33166
Fixes: #31860

PR-URL: #35431
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
This was referenced Feb 16, 2021
Closed
Merged
@targos targos added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 27, 2021
@targos targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vm.compileFunction is missing importModuleDynamically
7 participants
@devsnek @nodejs-github-bot @mcollina @SimenB @targos @ryzokuken @aduh95