module: use compileFunction over Module.wrap

[ryzokuken]

Use vm.compileFunction (which is a binding for
v8::CompileFunctionInContext) instead of Module.wrap internally in
Module._compile for the cjs loader.

Fixes: #17396

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

/cc @nodejs/modules-active-members @hashseed @addaleax

[nodejs-github-bot]

@ryzokuken build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/172/pipeline

[devsnek]

i assume you're still planning to add in the setters for _wrapper?

[ryzokuken]

@devsnek we could talk about that in this thread, I guess. I still am not 100% onboard on the idea that we'd have to somewhat complicate core just because people decided to monkey-patch internal core modules.

[devsnek]

@ryzokuken i generally feel the same way but this feels like a really big change...

@ChALkeR would you be able to run a gzemnid for Module._wrapper and Module.wrap?

[addaleax]

@ryzokuken I think it’s worth going that extra distance for compatibility

[ryzokuken]

@devsnek definitely +1 for gzemnid. I'd love to dive deeper into what the community is monkey-patching Module.wrapper for. If it's just the matter of changing the default params, I suppose that could be done by storing an array for the default params and making the setter set that array instead, while deprecating the setter itself to allow people to slowly migrate away.

If somebody is prepending code or something like that, I don't know how I feel about that though.

Maybe @jdalton will be able to give some insights on this?

[benjamingr]

Not sure what nodejs/modules can contribute - nodejs/modules is a team focused on CJS/ESM sompatibility and defining + landing stable ESM. (Not criticism of the ping - just setting expectations for the feedback the team can provide).

+1 on doing this by the way!

[jdalton]

As mentioned here (in a commit collapsed discussion) the use of Module.wrap and Module.wrapper to create the CJS module wrapping has existed since the start of 2011. I want to be careful here since the functionality being removed was easily discoverable and totally usable to customize the CJS wrapper.

I floated out a couple examples of how to tackle this. The one folks seemed to dig was putting setters on .wrap, .wrapper, wrapper[0], and .wrapper[1] that would revert to the old (current) behavior. That said, other approaches would be welcomed.

I'm all for this improvement, however I've repeatedly asked for caution around changing anything on the CJS-side. So I cannot approve this PR without steps taken to ensure interop and migration for CJS.

[ryzokuken]

@jdalton as I proposed,

I'd love to dive deeper into what the community is monkey-patching Module.wrapper for. If it's just the matter of changing the default params, I suppose that could be done by storing an array for the default params and making the setter set that array instead, while deprecating the setter itself to allow people to slowly migrate away.

I'm quite open to provide temporary setters and deprecating Module.wrap and Module.wrapper. That said, for making appropriate setters, I'd need to know why people are monkey-patching Module.wrap in the first place, and if it's reasonable enough for us to support that use case.

Stating my personal opinion, I'm actually not thrilled by the idea of supporting monkey-patches because I believe that they promote such reckless behavior, and it feels a lot like digging ourselves a hole, but I do understand that the CJS community might not have had too many options, and am willing to support the setters if the use-case is reasonable enough.

If you could help me with a few examples from userland (how and why exactly are people monkey-patching module), that'd be great.

[hashseed]

I think offering a legacy mode on the old code path is reasonable.

[ryzokuken]

List of code I found on GitHub that monkey-patched Module.wrap

(This list is WIP)

1. https://github.com/robertklep/top-level-await/blob/master/index.js#L5
   Changes function to async function in order to support top-level await.

2. https://github.com/isaacs/use-strict/blob/master/index.js
   Prepends 'use strict' to all your code.

3. https://github.com/bmeck/resource-shim/blob/2667b0be1666be8933bf1cdf1b82c4d629d6aec8/polyfill.js
   Seems to be prepending code to all your code as well.

[jdalton]

@ryzokuken

That said, for making appropriate setters, I'd need to know why people are monkey-patching Module.wrap in the first place, and if it's reasonable enough for us to support that use case.

CJS is incredibly flexible and tweakable (some say to a fault and others say to one of its biggest strengths). In the case here folks could tweak the wrapper of CJS to do things like make it an async function and allow top-level await for tests, remove-or-add provided vars, or add additional code to the wrapper for logging, strict mode, etc.

[ryzokuken]

@jdalton Adding strict mode, changing the default arguments, and adding top level await all seem not only common from my quick joyride throughout GitHub, I think they are reasonable enough to have fallback setters for them until we finally deprecate and remove Module.wrap altogether.

Regarding the ability to prepend/append arbitrary code though is something I have mixed thoughts on.

[bmeck]

Ignore the polyfill on mine, usage is ~0 and I don't encourage people to use it.

[jdalton]

@ryzokuken

I think they are reasonable enough to have fallback setters for them until we finally deprecate and remove Module.wrap altogether.

Rock!

[ryzokuken]

Ping everyone!

Now that #21571 has landed, let's get this done?

@jdalton could you help me make a list of use cases that we'd need to support?

[jdalton]

@ryzokuken

could you help me make a list of use cases that we'd need to support?

I see two parts of this. The Module.wrap() method and the Module.wrapper array. If there is customizing of either of the two then the legacy behavior should be used. Customizing means setting Module.wrap = ... or Module.wrapper = ... to something else. Or assigning new values to Module.wrapper[0] = ... or Module.wrapper[1] = ....

Note: Electron customizesModule.wrapper too. Since you're in the Electron org, you might head over there and work with them on an alternative approach.

[ryzokuken]

Note: Electron customizesModule.wrapper too. Since you're in the Electron org, you might head over there and work with them on an alternative approach.

Interesting! I'll talk to a few others in electron and work this out.

The Module.wrap() method and the Module.wrapper array. If there is customizing of either of the two then the legacy behavior should be used.

Sounds fair.

[jdalton]

@ryzokuken

Interesting! I'll talk to a few others in electron and work this out.

Related bits here and here.

[devsnek]

@jdalton since they're actually patching the source of core i don't think it is a problem.

[ryzokuken]

@jdalton @devsnek Exactly! They could just float another patch on top of their fork.

[jdalton]

Ideally Electron should patch Node directly less, so working with them on an alternative approach is a good thing. That said, I can sense this derailing here so feel free to take it up with me privately or create an Electron issue to track.

[bcoe]

@hashseed @addaleax @Trott, what's our reasoning for landing this in v11 but not v10 (if we're saying it's not a breaking change). I ask because it would make it makes it easier for c8 to support both versions of Node.js (since it wouldn't have to juggle two different wrapper lengths in 9 months when v10, v11, and v12 are the active branches).

[Trott]

@hashseed @addaleax @Trott, what's our reasoning for landing this in v11 but not v10 (if we're saying it's not a breaking change). I ask because it would make it makes it easier for c8 to support both versions of Node.js (since it wouldn't have to juggle two different wrapper lengths in 9 months when v10, v11, and v12 are the active branches).

If there's a compelling reason to land it in v10, I'm OK with that.

[BridgeAR]

@bcoe relying on the wrapper length is currently hard as any application could currently switch to using the old version in case any code monkey patches the wrapper / the wrap function. Both is quite likely.

We likely have overlooked that other applications might also rely on the wrapper length without manipulating the wrapper. AFAIK it is currently impossible to know what offset is actually used depending on the application code.

[bcoe]

@BridgeAR if the backport isn't too much of a pain, I'm just thinking that it's a lot less painful if V10 and V11 both use compileFunction and default to no longer having the wrapper.

If user-land modules then provide an alternative wrapper, it would be up to them to provide this information to other tools (perhaps via source-maps) -- the fact that v11/v12 differ in their behavior from v10, which will be the lts, seems like a potential headache going forward.... if the backport is an absolute pain, I understand not doing it.

[hashseed]

I don't have a strong opinion about back porting. I guess that depends on the complexity of the back port.

[guybedford]

AFAIK it is currently impossible to know what offset is actually used depending on the application code.

Perhaps we should add a Module.wrap.offset property?

If backporting will improve compatibility, then lets do it. When is the latest we can tackle the backport to ensure we have enough data on real-world usage here to be sure about any backport risk?