module: unflag --experimental-require-module

[joyeecheung]

This unflags --experimental-require-module so require(esm) can be used without the flag. For now, when require() actually encounters an ESM for the first time in a process, it will still emit an experimental warning. To opt out of the feature, --no-experimental-require-module can be used.

There are some tests specifically testing ERR_REQUIRE_ESM. Some of them are repurposed to test --no-experimental-require-module. Some of them are modified to just expect loading require(esm) to work, when it's appropriate.

This is expected to go out in 23 and get some testing before being backported to older LTS.

See #55085 (comment) for a summary of the impact of this on loading the high-impact npm packages.

Refs: #52697

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[nodejs-github-bot]

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

[joyeecheung]

Did some testing with https://github.com/joyeecheung/test-require-esm using the high-impact npm packages labeled as esm/dual/faux by https://github.com/wooorm/npm-esm-vs-cjs (I took the top 5000 as the sample), output is in https://gist.github.com/joyeecheung/f691e98e0994186f14417237ccb51f53 (note: I excluded a few packages that are not meant to be loaded in Node.js/not meant to be loaded as a module/cannot be installed properly due to conflicts with other packages out of the 5000 sample, see https://github.com/joyeecheung/test-require-esm/blob/a29118dbd1f868eddc64514c93a4b02c6c157013/try2.cjs#L7)

Summary:

Impact on dual packages

Before unflagging, on v22.9.0, 363 out of 379 dual packages could be require'd. After unflagging with this PR, 373 could be require'd - the +10 comes from fixing ERR_REQUIRE_ESM, I think they were just esm-only packages being mislabeled by npm-esm-vs-cjs as dual. Anyway, unflagging doesn't regress the loading the dual packages in this sample AFAICT.

Impact on esm-only packages

Before unflagging, on v22.9.0, 103 out of 662 esm packages could be require'd (again I think these were probably dual packages mislabeled as esm-only by npm-esm-vs-cjs). After unflagging, 639 out of 662 esm packages can be required. As far as I can tell the 23 that still cannot be loaded are expected:

1. 17 of they are defining only an import exports condition, forbidding require() to load them. If they want to open up to require(), they can simply change import to default so it can be loaded by both require() and import.
2. 6 of them are using top-level await. A few of them are only using it to dynamically load Node.js builtins, this can be fixed by switching to use globalThis.process.getBuiltinModule() that we introduced to eliminate the need of TLA for this.

The conclusion is, most (>95%) high-impact esm-only npm packages can now be loaded with require(esm) after the unflagging. Most of the packages that still can't be required can be fixed with small changes.

Impact on faux-esm packages

On both v22 and in this PR, 526 faux packages out of the 5000 high impact npm packages can be loaded. Only blob fails to load on both v22 (without the flag) and in this PR. blob could be loaded in v20, and this seems to be a regression already introduced in v22 caused by some interop issues with the esm package.

The conclusion is that this does not incur additional regression compared to v22, but there is likely a regression if backported as-is to v20. I don't think this prevents us from unflagging require(esm) in v23 given that it already exists in v22 without using the flag anyway, but we should at least look into the regression before we consider backporting this to v20.

Impact on cjs packages

Given that the majority of the high-impact npm packages are CommonJS so the number of them is the biggest, I didn't have the time to test them all yet, but some preliminary testing and the tests covered by the core test suites at least shows that they are unlikely to be affected by this change.

[joyeecheung]

I think this is ready for review - at least, for being landed in v23, PTAL @nodejs/tsc @nodejs/loaders

[mcollina]

lgtm

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @mcollina.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.24%. Comparing base (3c5ceff) to head (7af5b5d).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55085      +/-   ##
==========================================
- Coverage   88.24%   88.24%   -0.01%     
==========================================
  Files         651      651              
  Lines      183877   183868       -9     
  Branches    35858    35852       -6     
==========================================
- Hits       162266   162258       -8     
+ Misses      14901    14894       -7     
- Partials     6710     6716       +6     

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	97.21% <100.00%> (-0.17%)	⬇️
lib/internal/modules/esm/load.js	92.24% <ø> (-0.31%)	⬇️
lib/internal/modules/esm/loader.js	98.01% <100.00%> (-0.34%)	⬇️
lib/internal/modules/esm/module_job.js	100.00% <100.00%> (ø)
src/node_options.h	98.21% <100.00%> (ø)

... and 35 files with indirect coverage changes