nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.31k stars 29.46k forks source link

Roadmap for stabilization of vm modules #37648

Open targos opened 3 years ago

targos commented 3 years ago

According to @SimenB, here are the issues that should probably be taken care of before we unflag and mark vm modules as stable:

/cc @devsnek

SimenB commented 3 years ago

That last one might be a bug/race condition in Jest's implementation still. I haven't found the time to dig into it properly since opening up the issue, unfortunately. It feels like it's related to some module caching or context GC thing, but that's pure speculation. I have some hope it's related to whatever needs fixing for #36351 (again, speculating)

devsnek commented 3 years ago

definitely blocked on v8:9968

SimenB commented 3 years ago

The above linked v8 issue has been closed as fixed now (:tada:) - any idea of when we'll know if it's enough for Node's use cases?

targos commented 3 years ago

The commit fixing the above V8 issue is in V8 9.4. I'll probably open a PR upgrading to that version next week.

devsnek commented 3 years ago

it is not fixed quite yet, check v8:10284

SimenB commented 3 years ago

Ah ok, thanks for the link! Seems there's quite a bit of activity recently, so hopefully it's not too far away 🙂

vjeux commented 2 years ago

Just wanted to say that Jest is blocked on those tasks being completed in order to properly support ESM. It would be amazing if this could be prioritized!

jasonwilliams commented 2 years ago

Vitest is also blocked on those tasks too by the looks of https://github.com/vitest-dev/vitest/issues/795#issuecomment-1050129692. It seems to segfault on vm.runInThisContext when ran in debug mode on VSCode

benmccann commented 2 years ago

I got another segfault in Vitest: https://github.com/vitest-dev/vitest/issues/317

piranna commented 2 years ago

bytenode is also blocked by this.

targos commented 2 years ago

I think the root V8 bug is now chromium:1238312. There was recent activity on this CL: https://chromium-review.googlesource.com/c/v8/v8/+/3172764

devsnek commented 2 years ago

expansion doesn't work for chromium bugs, correct url is: https://bugs.chromium.org/p/chromium/issues/detail?id=1238312

v8 is working on overhauling host_defined_options to be a single Data instead of a FixedArray, as well as moving it to a common place that behaves better with caching for both modules and scripts (this includes the deprecation of v8::ScriptOrModule!!). i don't know what their planned timelines are but there's also some info here: https://bugs.chromium.org/p/chromium/issues/detail?id=1244145

targos commented 2 years ago

I added support for chromium: expansion.

SimenB commented 2 years ago

Should #44211 be added to the OP?

silverwind commented 1 year ago

vite also would have a use case for VM modules, if they were made stable.

HarikrishnanBalagopal commented 1 year ago

Any update on this? As mentioned earlier, using imports in jest test files needs the experimental flag

NODE_OPTIONS=--experimental-vm-modules jest

https://github.com/jestjs/jest/issues/12990#issuecomment-1182544621

GeoffreyBooth commented 1 year ago

Any update on this?

Pull requests are welcome!

AlbertMarashi commented 1 year ago

cmon.. it's been years. This feature would help w/ so much in the JS ecosystem

targos commented 1 year ago

Pull requests are still welcome.

SimenB commented 1 year ago

After 2.5 years, one of the issues in the OP can finally be checked off 😀

thernstig commented 1 year ago

@joyeecheung can the first item be ticked off since https://github.com/nodejs/node/issues/33439 is closed?

SimenB commented 12 months ago

Not sure if there's an issue for it, but the cache misses in vm.Script that was mitigated in #50137 (released in 21.1.0) will resurface when vm modules are unflagged - so fixing the cache in those cases should probably also be a blocker before unflagging/stabilizing.

metawrap-dev commented 6 months ago

Just to be clear, is this what is blocking VSCode extensions from using ES modules?

woss commented 6 months ago

@metawrap-dev no

RedYetiDev commented 5 months ago

@nodejs/vm I believe that https://github.com/nodejs/node/issues/36351 may be fixed (See https://github.com/nodejs/node/issues/36351#issuecomment-2094966217)

SimenB commented 3 months ago

2 of the linked issues in the OP have been closed - can probably tick them off yeah.

As for the remaining 2, #35714 is arguably a change that can be made as semver minor? And #33216 is hard to say whether is a bug in Node or in Jest, so probably shouldn't block stabilization.

Having --experimental-vm-modules be the default with a --no-experimental-vm-modules would be greatly beneficial, so with the blatent memory bugs fixed, maybe this can proceed?

tats-u commented 4 weeks ago

@targos Could you update the checklist?

https://github.com/nodejs/node/issues/33439 and https://github.com/nodejs/node/issues/36351 were closed.

targos commented 3 weeks ago

Updated

SimenB commented 3 weeks ago

Thoughts on making vm modules opt-out rather than opt-in for Node 23?

piranna commented 3 weeks ago

What do you mean? Can you elaborate?

thernstig commented 3 weeks ago

@piranna I would wager @SimenB means this: https://nodejs.org/api/vm.html#class-vmmodule

I.e. to not enforce --experimental-vm-modules but to enable it by default, but to opt out via a new flag instead if it is not wanted.

SimenB commented 3 weeks ago

Yeah 👍 Last sentence of https://github.com/nodejs/node/issues/37648#issuecomment-2222869414

Having --experimental-vm-modules be the default with a --no-experimental-vm-modules would be greatly beneficial, so with the blatent memory bugs fixed, maybe this can proceed?