-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v22.x] backport unflagging of --experimental-require-module and related fixes/refactorings #55217
[v22.x] backport unflagging of --experimental-require-module and related fixes/refactorings #55217
Conversation
Review requested:
|
CITGM for v22.x-staging: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3481/ |
Some regressions found:
I think for backporting to v22.x we should probably do some changes: a. Provides b would not fix the test regressions 2-4, the packages still need to update their tests which is to be expected. But at least it would not break the CLIs for end users. |
As for esm, from what I can tell in standard-things/esm#883 it seems to have been broken in different ways since many major releases of Node.js ago. The regression that predated unflagging of require(esm) I found in #55085 (comment) also seem to come from some TDZ issue in the way the package is written (it relies on access to Node.js internals and seem to expect certain internals to be invoked in certain order). It doesn't seem to be very cost-effective to do anything about it on v22. |
These SGTM |
Opened #55241 for feature detection. |
Added a commit from #55243 and a commit for v22.x specifically (and maybe v20.x in the future) to disable the warning when require() comes from node_modules. |
I just updated v22.x-staging so this now has conflicts because of duplicated commits I guess. |
dda6fc3
to
9c282c3
Compare
Rebased to drop the duplicate commits. Also cherry-picked #55243 and #55241 from the main branch. I think for backport to v22.x it is ready, note that the last commit (silence the experimental warning when it comes from node_modules) is only targeting v22.x, as proposed in #55217 (comment) and agreed in the last TSC meeting. |
9c282c3
to
3d0afe0
Compare
Can you rebase please? |
5ed6462
to
898396a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
898396a
to
5ea60dc
Compare
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: nodejs#55502 Fixes: nodejs#55500 Refs: nodejs#52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
Trim off irrelevant internal stack frames for require(esm) warnings so it's easier to locate where the call comes from when --trace-warnings is used. PR-URL: nodejs#55496 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
f7bdcee
to
a09b58c
Compare
As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: nodejs#55960 Refs: nodejs#55217 Refs: nodejs#52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: nodejs#55960 Refs: nodejs#55217 Refs: nodejs#52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
a09b58c
to
4af41f0
Compare
Updated the branch and reordered the commits a bit to match the main branch. |
Thank you @joyeecheung! |
As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: nodejs#55960 Refs: nodejs#55217 Refs: nodejs#52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: #55960 Refs: #55217 Refs: #52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[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: #55217 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]>
PR-URL: #54563 Backport-PR-URL: #55217 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Refs: #52697
PR-URL: #55199 Backport-PR-URL: #55217 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Refs: #52697
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: #55243 Backport-PR-URL: #55217 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Refs: #52697
Previously we assumed if `--experimental-detect-module` is true, then `--experimental-require-module` is true, which isn't the case, as the two can be enabled/disabled separately. This patch fixes the checks so `--no-experimental-require-module` is still effective when `--experimental-detect-module` is enabled. Drive-by: make the assertion messages more informative and remove obsolete TODO about allowing TLA in entrypoints handled by require(esm). PR-URL: #55250 Backport-PR-URL: #55217 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Refs: #52697
This previously compiles a script and run it in a new context to avoid global pollution, which is more complex than necessary and can be too slow for it to be reused in other cases. The new implementation just checks the frames in C++ which is safe from global pollution, faster and simpler. The previous implementation also had a bug when the call site is in a ESM, because ESM have URLs as their script names, which don't start with '/' or '\' and will be skipped. The new implementation removes the skipping to fix it for ESM. PR-URL: #55286 Backport-PR-URL: #55217 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Refs: #52697
When emitting the experimental warning for `require(esm)`, include information about the parent module and the module being require()-d to help users locate and update them. PR-URL: #55397 Backport-PR-URL: #55217 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Refs: #52697
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: #55502 Backport-PR-URL: #55217 Fixes: #55500 Refs: #52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
Trim off irrelevant internal stack frames for require(esm) warnings so it's easier to locate where the call comes from when --trace-warnings is used. PR-URL: #55496 Backport-PR-URL: #55217 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Refs: #52697
As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: #55960 Backport-PR-URL: #55217 Refs: #55217 Refs: #52697 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
Landed in 9ffd2dd...cb527a9 |
This backports the following PRs that are related to the unflagging of --experimental-require-module:
Note that I don't think this backport PR is strictly required - the PRs seem to land cleanly on v22.x except the unflagging commit has a small conflict in the YAML changelog description. This PR is mostly opened to look for regressions in v22 with CITGM.
Refs: #52697