module: support 'module.exports' interop export name in require(esm)

[guybedford]

This adds support for export { foo as 'module.exports' } in a CJS module to imply that this export value should always be used as the module value when requiring that ESM module under require(esm), allowing other types than just module namespaces in this interop.

This effectively splits off the non-major change from #53848 as discussed in the Node.js loaders meeting, under a rename of this flag to use the 'module.exports' pattern, where that PR will be updated to use the name that lands here on the ESM wrapper as a major change further.

The naming bikeshed took place in nodejs/loaders#221 and was determined by TSC vote.

//cc @nodejs/loaders

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (89a2f56) to head (78d41a2).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #54563   +/-   ##
=======================================
  Coverage   88.39%   88.40%           
=======================================
  Files         652      652           
  Lines      186565   186568    +3     
  Branches    36046    36043    -3     
=======================================
+ Hits       164916   164929   +13     
- Misses      14908    14911    +3     
+ Partials     6741     6728   -13     

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	97.42% <100.00%> (+<0.01%)	⬆️

... and 23 files with indirect coverage changes

[guybedford]

//cc @nodejs/loaders this PR is ready for final review.

[mcollina]

I really wish we used __cjsUnwrapDefault instead, but I'm ok with this as well.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[joyeecheung]

There is another caveat of the default live-binding being broken (compared to ESM import semantics). Though I guess that's rather an edge case that most won't care about - this feature is intended for library authors who migrate from CommonJS, which doesn't support re-assigning module.exports once it's cached anyway.

[ljharb]

default exports in ESM do not have live bindings, only named exports do.

[joyeecheung]

default exports in ESM do not have live bindings, only named exports do.

I am talking about this (which can't be mimicked with native CommonJS, but then it has never been, so for CommonJS -> ESM module migration, it doesn't matter as the original CommonJS module is unlikely to expect re-assigning module.exports to be effective in consumer code. It's only a problem for existing ESM that re-assigns default exports and tries to extend support to CommonJS consumers, which should be rare):

// a.mjs
let a = 1;
export function increment() {
  a++;
}
export { a as default };

// b.mjs
import a, { increment } from './a.mjs';
increment();
console.log(a);  // 2

[ljharb]

Oh interesting, you’re saying that when exported as a name, it does have live binding behavior - TIL.

[joyeecheung]

We have an alternative proposal in nodejs/loaders#221 which people seem to like over there:

function foo() {}

export { foo as "module.exports" };
export default foo;

I think we should do a poll of this v.s. what this PR currently proposes:

function foo() {}

export const __cjsUnwrapDefault = true;
export default foo;

and decide which one we should go with. I am not sure what the poll audience should be though, should it be:

• TSC, as usual
• collaborators, because this is a naming choice and seems fun to vote on
• Broader user base (social media poll?)

placing a block until we have a decision.

[guybedford]

@joyeecheung given there's quite a bit of nuance here, I'd suggest we discuss this within the TSC. I'll make myself available for the next meeting on the 11th. I'd like us to come to a conclusion as swiftly as possible though and encourage a vote / decision in the meeting, as this has discussion has now dragged on since July 14. I'll commit to whatever decision is made there for this PR and align that result with #53848.

[jasnell]

I prefer the more explicit use of __cjsUnwrapDefault = true personally as it seems less magical and more explicit about the intent.

[guybedford]

I believe all outstanding comments have been addressed. It's been a ride, we might finally be there.

Will aim to land shortly unless anyone would like to engage in further debate.

[nodejs-github-bot]

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

[guybedford]

Landed in d24c731.