module: disable cjs snapshotting into esm loader

[guybedford]

This disables the instant snapshotting of CJS modules into the ESM loader as soon as they are loaded, regardless of whether those modules are loaded as ESM.

This allows loaders to support named exports for CJS modules, which isn't currently possible with the loader hook module as the snapshot process bypasses the loader hooks.

Since the PR at #33416 for CommonJS named exports has stalled due to a remaining blocker, this will allow loaders to enable that use case.

This was originally implemented in order to ensure that the snapshot of the module.exports value was always a sync exact snapshot at execution completion. In addition this allows CJS modules to be more easily garbage collected since they are not by default injected into the ESM loader if not directly loaded from it.

Checklist

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

[guybedford]

/cc @nodejs/modules-active-members this was as discussed at the last meeting.

[bmeck]

The issue is that this means that every single CJS module loaded is being bound into the ESM registry and can never be garbage collected.

Isn't this true for anything that is set to an exported name, even without this PR?

[guybedford]

Isn't this true for anything that is set to an exported name, even without this PR?

Yes, if they are explicitly imported into the ES module loader from the CommonJS loader. Most CommonJS modules are not though.

[bmeck]

Overall if we can get an example of a named exports loader I agree with the concept of this, but might be more inclined to a different API. Removal of CommonJS values often don't GC anyway due to require.cache so I'd state that as being less compelling.

[devsnek]

i'm not a fan of this. worth exploring better module gcing of course, but i think the snapshots are an important behavior we should keep.

[jkrems]

I considered the snapshots an awkward mechanism for a while now. It seems like a leaky abstraction when the CJS system populates ESM caches. I get the theoretical appeal but I don't think it's worth the workarounds, especially when they prevent things like user-land exploration of alternate CJS loading strategies.

[guybedford]

@bmeck I've updated the description to reflect the fact that this is first and foremost about allowing named exports loaders.

I'd be glad to put such a loader together for this PR, I just am watching series right now and trying to forget my life problems.

[guybedford]

@bmeck as requested, here is a CJS named exports loader that will work with this PR approach - https://github.com/guybedford/cjs-named-exports-loader.

[nodejs-github-bot]

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

[guybedford]

Landed in 1fe39f0.

[devsnek]

Please revert this. I did not approve of it landing.

[bmeck]

@devsnek please the the request changes feature in such a case. this was discussed at the modules WG yesterday as well as being left open with approvals. we can queue it to the modules wg again I guess.

[devsnek]

I believe snapshots, because of the predictability they provide, are a useful invariant of our esm/cjs interop. I'm happy to discuss technical details of export detection and gcing and whatnot as I don't think this feature fundamentally blocks those from working.

[jasnell]

Reopening this since it was reverted...

[jasnell]

Marking this with a Request Changes to keep it from landing again before the issues can be worked through

[guybedford]

I do want to be explicit in closing this - we either snapshot the CommonJS module.exports values on startup or we do not, and with the direct block on the very concept there's no other way around that question.

Per my comment in #34562 (review) there will be other ways to tackle the CJS GC question if anyone wants to dig into that, but that would be separate work from this.

For the named exports feature we can work around this invariant instead.