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.

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

vm.compileFunction is missing importModuleDynamically #31860

Closed
SimenB opened this issue Feb 19, 2020 · 3 comments · Fixed by #32985 or #35431
Closed

vm.compileFunction is missing importModuleDynamically #31860

SimenB opened this issue Feb 19, 2020 · 3 comments · Fixed by #32985 or #35431
Assignees
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@SimenB
Copy link
Member

SimenB commented Feb 19, 2020

Is your feature request related to a problem? Please describe.

I'm currently working on implementing support for native ES Modules in Jest. While both vm.SourceTextModule and vm.Script allows us to define an importModuleDynamically option, this is missing from vm.compileFunction. The option is also present in runInContext and runInThisContext.

(I'm aware that vm's ESM APIs are experimental - I'm just playing with the APIs to see if they fit with Jest's use cases for them. It's currently looking really good! 🙂)

Describe the solution you'd like

The option should be added to compileFunction as well, to have parity with the other vm functions/constructs.

Describe alternatives you've considered

Not using compileFunction and sticking to vm.Script for CommonJS. I'd really like to avoid that as we just migrated to compileFunction which made V8 code coverage mapping from source maps way easier than the old manual module wrapper we use in vm.Scripts. Also, it's a bit cleaner on our side when we can avoid manually creating the module wrapper.

@devsnek devsnek self-assigned this Feb 19, 2020
@devsnek devsnek added the vm Issues and PRs related to the vm subsystem. label Feb 19, 2020
@SimenB
Copy link
Member Author

SimenB commented Apr 21, 2020

@devsnek hey 👋 Starting to land ESM support in Jest now, and was looking into supporting import() in CJS, and hit this again (forgot I filed it 😅). I would offer to help, but I've never done any C++ except for simplistic school assignments some years ago. Looking at #22381 it seems like it's almost entirely C++, but if I can do anything else to help I'd love to!

(As an aside, Jest 25.4 includes primitive support for ESM - I'm really digging the APIs! Thanks for all your hard work on this.)

@SimenB
Copy link
Member Author

SimenB commented Apr 25, 2020

Ah thank you!

BethGriggs pushed a commit that referenced this issue Apr 27, 2020
Fixes: #31860

PR-URL: #32985
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BridgeAR pushed a commit that referenced this issue Apr 28, 2020
Fixes: #31860

PR-URL: #32985
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BridgeAR pushed a commit that referenced this issue Apr 28, 2020
Fixes: #31860

PR-URL: #32985
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BethGriggs pushed a commit that referenced this issue Apr 28, 2020
Fixes: #31860

PR-URL: #32985
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@mcollina mcollina reopened this May 20, 2020
@mcollina
Copy link
Member

Reopening as this was reverted due to #33166.

devsnek added a commit to devsnek/node that referenced this issue Sep 29, 2020
devsnek added a commit to devsnek/node that referenced this issue Feb 5, 2021
devsnek added a commit to devsnek/node that referenced this issue Feb 5, 2021
@devsnek devsnek closed this as completed in bf2f2b7 Feb 5, 2021
danielleadams pushed a commit that referenced this issue Feb 16, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
3 participants