module: unflag --experimental-require-module #55085
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
joyeecheung
added
semver-minor
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
avivkeller
added
esm
joyeecheung
force-pushed
the
unflag-require-module
branch
from
8c00119 to
7af5b5d
Compare
github-actions
bot
removed
the
request-ci
|
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 packagesBefore unflagging, on v22.9.0, 363 out of 379 dual packages could be Impact on esm-only packagesBefore unflagging, on v22.9.0, 103 out of 662 esm packages could be
The conclusion is, most (>95%) high-impact esm-only npm packages can now be loaded with Impact on faux-esm packagesOn both v22 and in this PR, 526 faux packages out of the 5000 high impact npm packages can be loaded. Only 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 Impact on cjs packagesGiven 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 |
mcollina
added
the
notable-change
|
The
notable-change
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 ReportAll modified and coverable lines are covered by tests ✅
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
|
|
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. |
|
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 I can fix this by changing the config file extension from 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 |
Different kind of issue, but it does look fairly common to do a fallback based on whether the thrown exception is 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 That said: It’s possible for these modules to always use |
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 <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
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 <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
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 <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
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 <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
joyeecheung
added
backport-open-v20.x
|
Backport in #56927 |
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 <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
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 <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
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 <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
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: #55085 Backport-PR-URL: #56927 Refs: #52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Notable changes: crypto: * update root certificates to NSS 3.107 (Node.js GitHub Bot) #56566 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) unflag --experimental-require-module (Joyee Cheung) #55085 * (SEMVER-MINOR) implement the "module-sync" exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 * (SEMVER-MINOR) add __esModule to require()'d ESM (Joyee Cheung) #52166 process: * (SEMVER-MINOR) add process.features.require_module (Joyee Cheung) #55241 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: TODO
Notable changes: crypto: * update root certificates to NSS 3.107 (Node.js GitHub Bot) #56566 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) unflag --experimental-require-module (Joyee Cheung) #55085 * (SEMVER-MINOR) implement the "module-sync" exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 * (SEMVER-MINOR) add __esModule to require()'d ESM (Joyee Cheung) #52166 process: * (SEMVER-MINOR) add process.features.require_module (Joyee Cheung) #55241 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: #57349
Notable changes: crypto: * update root certificates to NSS 3.107 (Node.js GitHub Bot) #56566 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) unflag --experimental-require-module (Joyee Cheung) #55085 * (SEMVER-MINOR) implement the "module-sync" exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 * (SEMVER-MINOR) add __esModule to require()'d ESM (Joyee Cheung) #52166 process: * (SEMVER-MINOR) add process.features.require_module (Joyee Cheung) #55241 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: #57349
Notable changes: crypto: * update root certificates to NSS 3.107 (Node.js GitHub Bot) #56566 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) unflag --experimental-require-module (Joyee Cheung) #55085 * (SEMVER-MINOR) implement the "module-sync" exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 * (SEMVER-MINOR) add __esModule to require()'d ESM (Joyee Cheung) #52166 process: * (SEMVER-MINOR) add process.features.require_module (Joyee Cheung) #55241 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: #57349
Notable changes: crypto: * update root certificates to NSS 3.107 (Node.js GitHub Bot) #56566 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) unflag --experimental-require-module (Joyee Cheung) #55085 * (SEMVER-MINOR) implement the "module-sync" exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 * (SEMVER-MINOR) add __esModule to require()'d ESM (Joyee Cheung) #52166 process: * (SEMVER-MINOR) add process.features.require_module (Joyee Cheung) #55241 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: #57349
Notable changes: crypto: * update root certificates to NSS 3.107 (Node.js GitHub Bot) #56566 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) unflag --experimental-require-module (Joyee Cheung) #55085 * (SEMVER-MINOR) implement the "module-sync" exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 * (SEMVER-MINOR) add __esModule to require()'d ESM (Joyee Cheung) #52166 process: * (SEMVER-MINOR) add process.features.require_module (Joyee Cheung) #55241 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: #57349
Notable changes: crypto: * update root certificates to NSS 3.107 (Node.js GitHub Bot) #56566 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) unflag --experimental-require-module (Joyee Cheung) #55085 * (SEMVER-MINOR) implement the "module-sync" exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 * (SEMVER-MINOR) add __esModule to require()'d ESM (Joyee Cheung) #52166 process: * (SEMVER-MINOR) add process.features.require_module (Joyee Cheung) #55241 worker: * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682 PR-URL: #57349
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