module: unflag --experimental-require-module #55085

joyeecheung · 2024-09-23T17:35:58Z

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 · 2024-09-23T17:36:03Z

Review requested:

@nodejs/loaders

lib/internal/modules/esm/loader.js

test/es-module/test-cjs-esm-warn.js

test/es-module/test-esm-type-field-errors-2.js

test/es-module/test-require-module-preload.js

test/parallel/test-require-mjs.js

nodejs-github-bot · 2024-09-24T09:28:11Z

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

joyeecheung · 2024-09-24T11:40:52Z

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:

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.
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 · 2024-09-24T11:50:05Z

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

mcollina

lgtm

github-actions · 2024-09-24T12:04:54Z

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 · 2024-09-24T13:29:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.23%. Comparing base (5c22d19) to head (eaa5d48).
Report is 111 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

nodejs-github-bot · 2024-09-26T14:21:39Z

Landed in 06206af

joyeecheung · 2024-09-30T12:07:03Z

Putting it on the TSC agenda to discuss the release plan of the unflagging.

This unflags --experimental-require-module so require(esm) can be used without the flag. For now, when require() actually encounters an ESM, 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. PR-URL: nodejs#55085 Refs: nodejs#52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Richard Lau <rlau@redhat.com>

This unflags --experimental-require-module so require(esm) can be used without the flag. For now, when require() actually encounters an ESM, 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. PR-URL: nodejs#55085 Backport-PR-URL: nodejs#55217 Refs: nodejs#52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Richard Lau <rlau@redhat.com>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 23, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024

This comment was marked as outdated.

Sign in to view

RedYetiDev reviewed Sep 24, 2024

View reviewed changes

joyeecheung force-pushed the unflag-require-module branch from 8c00119 to 7af5b5d Compare September 24, 2024 09:23

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

joyeecheung marked this pull request as ready for review September 24, 2024 11:47

mcollina approved these changes Sep 24, 2024

View reviewed changes

mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Sep 24, 2024

marco-ippolito approved these changes Sep 24, 2024

View reviewed changes

RafaelGSS approved these changes Sep 24, 2024

View reviewed changes

anonrig approved these changes Sep 24, 2024

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 26, 2024

nodejs-github-bot merged commit 06206af into nodejs:main Sep 26, 2024
67 checks passed

imathews mentioned this pull request Sep 26, 2024

Provide ESM build? nene/sql-parser-cst#87

Open

fengmk2 mentioned this pull request Sep 28, 2024

使用 tshy 来默认支持 cjs 和 esm eggjs/egg#5257

Open

pirxpilot mentioned this pull request Sep 28, 2024

Ts esm commonjs support pirxpilot/connect-gzip-static#17

Closed

joyeecheung added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 30, 2024

This was referenced Sep 30, 2024

Node.js Technical Steering Committee (TSC) Meeting 2024-10-02 nodejs/TSC#1626

Closed

Many modules labeled as esm can be loaded by require() (prior to require(esm)) wooorm/npm-esm-vs-cjs#11

Open

joyeecheung mentioned this pull request Oct 1, 2024

[v22.x] backport unflagging of --experimental-require-module and related fixes/refactorings #55217

Open

tats-u mentioned this pull request Oct 4, 2024

Enable consuming of ES modules in extensions microsoft/vscode#130367

Open

joyeecheung added backport-open-v22.x Indicate that the PR has an open backport and removed dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Oct 7, 2024

jonkoops mentioned this pull request Oct 10, 2024

refactor!: drop ES5 distribution from package react-dropzone/file-selector#92

Merged

12 tasks

joyeecheung mentioned this pull request Oct 10, 2024

module: implement the "module-sync" exports condition #54648

Merged

RafaelGSS mentioned this pull request Oct 11, 2024

2024-10-15, Version 23.0.0 (Current) #55338

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: unflag --experimental-require-module #55085

module: unflag --experimental-require-module #55085

joyeecheung commented Sep 23, 2024 •

edited

Loading

nodejs-github-bot commented Sep 23, 2024

This comment was marked as outdated.

nodejs-github-bot commented Sep 24, 2024

joyeecheung commented Sep 24, 2024 •

edited

Loading

joyeecheung commented Sep 24, 2024

mcollina left a comment

github-actions bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

nodejs-github-bot commented Sep 26, 2024

joyeecheung commented Sep 30, 2024

module: unflag --experimental-require-module #55085

module: unflag --experimental-require-module #55085

Conversation

joyeecheung commented Sep 23, 2024 • edited Loading

nodejs-github-bot commented Sep 23, 2024

This comment was marked as outdated.

nodejs-github-bot commented Sep 24, 2024

joyeecheung commented Sep 24, 2024 • edited Loading

Summary:

Impact on dual packages

Impact on esm-only packages

Impact on faux-esm packages

Impact on cjs packages

joyeecheung commented Sep 24, 2024

mcollina left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Sep 26, 2024

joyeecheung commented Sep 30, 2024

joyeecheung commented Sep 23, 2024 •

edited

Loading

joyeecheung commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading