re-add VM modules to our fork

[MylesBorins]

Based on today's call does it make sense for us to rebase out the removal of VM modules?

[guybedford]

Sounds good to me.

[jdalton]

I still lean -1 on inclusion.

[devsnek]

i'm in favour of including it. it's totally safe to make breaking changes to it, as it's under --experimental-vm-modules

[MylesBorins]

@jdalton can you please expand on your objection based on the consensus we had in the meeting earlier?

[jdalton]

AFAIK we had no formal consensus to add back the VM module. I do not feel the VM additions are a hard requirement to ship in April and think they can live in other places/repos within the Node org if needed.

[MylesBorins]

@jdalton to be explicit my ask is to remove the commit that "removes" VM modules and move that work to core. Essentially freeing us from the responsibility.

[jdalton]

I prefer it stay removed.

[devsnek]

@jdalton vm.Module was added separately from the goals of this group, for uses outside of the goals of this group. I'm ok with the API being in flux and being modified to suit the internal design of our system, but it has to remain when this process is over, outside of any decisions this specific group makes.

Because of this, I think it is a lot easier for us to work with it rather than pretend it doesn't exist. If we pretend it doesn't exist, the burden falls on me or one of the other vm maintainers to muck through months of changes to internals to add it back.

[jdalton]

@devsnek

vm.SourceTextModule was added separately from the goals of this group, for uses outside of the goals of this group.

IMO the vm.SourceTextModule should not exist without our module implementation and is directly impacted by our design choices. It feels like a disconnect to consider it as something completely separate and outside the goals of this group. IMO It's within the scope of the WG, additive, but not required for April.

[devsnek]

@jdalton i agree that it's connected, my point is that it exists whether or not node runs esm, as the reasons it was added in fact had nothing to do with node running esm files. if you want to remove it from core, you'll need to work with people in core to do so, not this group specifically.

[jdalton]

Our existing --experimental-modules and --experimental-vm-modules are subject to radical change depending on what lands in April. My preference is for its continued removal from our fork.

[devsnek]

@jdalton sure, i'm not against radical change, i'm just strongly advising against not having vm modules, because its out of the scope of this group to remove it, the same way its out of the scope of this group to remove fs.promises.

In april if we want to remove vm.SourceTextModule from core entirely we need to have consensus to do so from the vm team.

[jdalton]

@devsnek Thank you for your point of view. I see it differently is all.
Failing consensus, if it falls to a vote in the group that's okay.

[zenparsing]

@devsnek

the reasons it was added in fact had nothing to do with node running esm files

Can you tell me more about this?

[devsnek]

@zenparsing Simply put, vm.SourceTextModule was added to fulfill the design goals of VM, which are exposing APIs to evaluate JS. From the docs of VM:

VM (Executing JavaScript)
The vm module provides APIs for compiling and running code within V8 Virtual Machine contexts.

The specific design of the vm.SourceTextModule API was shaped by the spec text, and by some input from JSDOM.

[zenparsing]

@devsnek Thanks. Do you know if the input from jsdom is recorded anywhere (e.g. the jsdom repo or the node repo)?

[devsnek]

@zenparsing i don't have logs, no, but really the only two things were 1) able to use in main context and 2) able to create more than one map per context.

The second one is also a constraint of the general api, since two separate libraries operating in the same context (even if it isn't the main context) shouldn't interfere with each other.

[guybedford]

It's worth being careful here not to overlook the repercussions on this from a Node.js core perspective. Already, Node.js core is having backporting issues to previous Node majors due to the vm implementation, and this is a large part of the reason why the rebasing efforts on this repo are becoming so difficult.

I quite like the idea of handing the responsibility of vm back to core as it reduces the scope of this group and removes those blocks.

At the same time I can appreciate the reservations too.

[GeoffreyBooth]

Should this really be a separate thing? Aren’t VM modules, um, very closely related to modules? If not dependent on our modules implementation?

If VM modules is dependent on the ES modules implementation, then it seems to me that either we need to update VM modules when we replace --experimental-modules with our new implementation; or remove VM modules when we swap out --experimental-modules, and add VM modules back in when a new VM modules implementation is written that works with the new ES modules implementation.

[devsnek]

@GeoffreyBooth vm.SourceTextModule and node's loader use the same infrastructure internally, although they are completely separate externally. When making changes to what we're working on, it's likely you will also have to make changes to vm.SourceTextModule.

Whether we keep vm.SourceTextModule in the kernel and keep it updated as we move, or add it back in all at once when we merge back into core is up to this group, but by the time we're putting our work in core, vm.SourceTextModule needs to be accounted for.

[GeoffreyBooth]

Whether we keep vm.SourceTextModule in the kernel and keep it updated as we move, or add it back in all at once when we merge back into core is up to this group, but by the time we’re putting our work in core, vm.SourceTextModule needs to be accounted for.

This seems to me more of a resourcing question, as in: do we have the bandwidth to update VM modules along with creating the new ES modules implementation? If we do, sure, let’s merge in an updated VM modules along with replacing --experimental-modules with our new implementation. But if an updated VM modules isn’t ready before the March/April cutoff, it’s probably better to just remove VM modules in order to get the new ES modules implementation in, and the updated VM modules can follow on as soon as it’s ready.

[devsnek]

do we have the bandwidth to update VM modules along with creating the new ES modules implementation?

From personal experience keeping both working, I would say absolutely yes. The worst case scenario is that you have to tweak the public API of vm.SourceTextModule, and that's still just changing some JS and updating some docs. The entire source of vm.SourceTextModule[1] is a single 250sloc file, so it's not usually a rabbit hole situation.

I'm always hanging around and willing to fix anything that breaks, but if you or @guybedford are worried about this complexity we can come up with something else. The bottom line is that when the ecmascript-modules repo gets merged into core, if vm.SourceTextModule is removed, I will block the PR until I can write up the changes needed to re-enable it (yes, i will personally write up the changes 😄, but i would rather we just keep it incrementally updated).

1: https://github.com/nodejs/node/blob/master/lib/internal/vm/source_text_module.js

[MylesBorins]

Based on the consensus from the last meeting I think we should be able to rebase out the commit that removes VM modules. It appears that our current branch has conflicts so I'll do so in the rebase PR