Skip to content

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

Merged
devsnek merged 1 commit intonodejs:masterfrom
devsnek:revert-revert-ugh
Feb 5, 2021
Merged

Revert "Revert "vm: add importModuleDynamically option to compileFunction""#35431
devsnek merged 1 commit intonodejs:masterfrom
devsnek:revert-revert-ugh

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

@devsnek devsnek requested a review from mcollina September 30, 2020 15:46
@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

nodejs-github-bot commented Sep 30, 2020

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

mcollina commented Sep 30, 2020

@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

nodejs-github-bot commented Sep 30, 2020

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

mcollina commented Sep 30, 2020

cc @targos

@mcollina
Copy link
Member

mcollina commented Oct 23, 2020

@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

mcollina commented Oct 24, 2020

@devsnek could you rebase?

@SimenB
Copy link
Member

SimenB commented Oct 24, 2020

Note that if you're testing with jest@26.5.0 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

mcollina commented Oct 24, 2020

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
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 5, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 5, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 5, 2021

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

nodejs-github-bot commented Feb 5, 2021

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 <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
@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 <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
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

@mcollina mcollina mcollina approved these changes

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

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