Review requested:

• @nodejs/loaders

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

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.

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

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 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.

@@            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     

... and 38 files with indirect coverage changes

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.

@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.

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)

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

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.

Backport in #56927