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.

For more background on the motivation of require(esm) see #51977 - TL;DR: it helps accelerating ESM adoption in the ecosystem as package authors can start shipping native ESM with less breakage to their CJS users; it also helps frameworks and tools that take plugins to support native ESM in user/plugin code whilst they are still navigating their own migration to ESM.

Note to releasers: in the release announcements we should emphasize the implications this has on top-level await is limited to require(). In entry points or modules that are only ever imported, top-level await works fine as before. Only when one tries to use the synchronous require() to evaluate a module that awaits at module loading time (top-level), it obviously would not work, not that it ever worked before require() supports ESM, just that it's now the only significant remaining exception for require(esm).

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.23%. Comparing base (5c22d19) to head (eaa5d48).
Report is 947 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55085      +/-   ##
==========================================
- Coverage   88.25%   88.23%   -0.02%     
==========================================
  Files         651      651              
  Lines      183856   183845      -11     
  Branches    35850    35841       -9     
==========================================
- Hits       162259   162222      -37     
- Misses      14888    14907      +19     
- Partials     6709     6716       +7     

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	97.26% <100.00%> (-0.11%)	⬇️
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%> (ø)
lib/internal/modules/esm/utils.js	98.88% <100.00%> (-0.01%)	⬇️
src/node_options.cc	88.04% <ø> (ø)
src/node_options.h	98.21% <100.00%> (ø)

... and 38 files with indirect coverage changes

[joyeecheung]

We did not remove the flag, only changed its default to true and thus enabled it by default, which is the equivalent of adding a feature by default and is considered as semver-minor after discussions in the TSC.

Note that semver doesn't mean all non-major changes are intended to be non-breaking in any possible way. Technically, there is almost no change that is not a breaking change, a bugfix can be unintentionally breaking for some code if code already starts to rely on the bug. But based on what we observed so far, this should be considered semver-minor. Most of the unexpected breakages have to do with monkey-patching of Node.js internals which we already provide no compatibility guarantees for and could easily be broken in any internal refactoring released as semver patch.

[DavidAnson]

The new console output produced by this experimental feature will fail CI for markdownlint-cli2 on main branch (unchanged in a month) once GitHub starts using the corresponding Node version: https://github.com/DavidAnson/markdownlint-cli2/actions/workflows/ci.yml?query=branch%3Amain

So far as I know, that project uses everything in a valid, documented, and supported manner. In the before times, it seemed necessary to guess-and-check for an arbitrary module reference to know how to load it (require vs. import). That behavior now produces visible output to the console which is undesirable for a CLI tool.

It may be that my scenario is sufficiently weird/rare/broken/dumb that nobody cares and that's fine. I'll deal with this. However, my claim is that opting everyone into an experimental change to fundamental loader behavior is not a minor change. In my opinion, version X.12 of an LTS release is not the time to be opting the community in to new experiments.

[joyeecheung]

The warning is only emitted if it's coming from somewhere outside node_modules, by the way. For your own CI you can consider supressing it with --disable-warning=experimentalWarning, or NODE_NO_WARNINGS=1.

In my opinion, version X.12 of an LTS release is not the time to be opting the community in to new experiments.

You are only opting into it if you were previously assuming such feature does not exist and attempting to use it will always throw, but the same logic can apply to many other semver-minor features. If you consider changing "not supporting a feature" to "now supporting a feature but experimental" to be breaking, then semver-minor probably does not exist.

In any case, I am not sure what's to be done here - it had been discussed in the TSC and was considered semver-minor, Many users have asked for backporting it to LTS as semver-minor so that people can start shipping ESM-only packages without breaking losing reach to CJS users. It would be rather difficult to go back and tell them we will remove it in LTS and people will wait for another 4 years or so if they wish to ship ESM for all active LTS.

[DavidAnson]

Smarter people than I have decided this is the best path forward, and I'm not asking Node to change for me. :)

That said, I think there is a clear difference between (for example) adding a new function that has zero impact to published packages (semver minor) and changing an existing function which has known, observable impact to published packages (semver major). And I think it's significant that the change is still experimental.

I appreciate your time here and you don't need to try to convince me further. :) I expect I can accommodate the new behavior and publish a new package before this affects too many folks.

Thank you.

[xt0rted]

This change silently broke one of our build steps and was a pain in the butt to track down. I ended up tracing the issue back to how tailwindcss loads plugins.

My package.json is set to type module, my tailwind config uses .js as the extension, and on v22.11.0 everything works fine, but in v22.12.0 and v23.* the @tailwindcss/forms plugin is no longer loaded, there are no errors, and the resulting css output is incorrect.

I can fix this by changing the config file extension from .js to .mjs or by modifying my script like so cross-env NODE_OPTIONS=\"--no-experimental-require-module\" tailwindcss ....

Despite finding those fixes, which feel more like hacks, there were no errors or warnings in the console, and node itself didn't emit anything. This was working fine for me since I had 22.11.0, but a coworker was having issues which were traced back to having 22.12.0 installed.

[joyeecheung]

@xt0rted You are likely seeing a similar issue from #56155 (comment), likely caused by using/patching of Node.js internals and making unreliable assumptions about how Node.js internals work. If there is a way to reproduce it without using third-party code or undocumented internals maybe we could fix it in Node.js, otherwise it's likely a bug in tailwind or jiti and needs to be fixed on their side.

[joyeecheung]

FYI it seems the issue in tailwindcss was caused by some unreliable assumptions jiti makes about Node.js internals and probably has been fixed in the latest releases of jiti: unjs/jiti#346 (comment)

[DavidAnson]

FYI, it seems like webpack was affected by this change as well: webpack/webpack-cli#4340

[voxpelli]

FYI, it seems like webpack was affected by this change as well: webpack/webpack-cli#4340

Different kind of issue, but it does look fairly common to do a fallback based on whether the thrown exception is ERR_REQUIRE_ESM: https://github.com/search?q=ERR_REQUIRE_ESM+language%3AJavaScript&type=code&l=JavaScript

And the one case that still would need that fallback, the top level awaits, will not get that fallback as the thrown exception is now ERR_REQUIRE_ASYNC_MODULE instead.

That said: It’s possible for these modules to always use await import() if they separately resolve the path using the require algorithm first.

[joyeecheung]

Backport in #56927