GeezerVison

Copy link

Copy Markdown

Member

kirillgroshkov, snewell92, styfle, sedghi, and MomenNano reacted with hooray emoji bricss, acommodari, marco-ippolito, jonkoops, kirillgroshkov, snewell92, styfle, and MomenNano reacted with rocket emoji

joyeecheung force-pushed the backport-require-esm-2 branch from dda6fc3 to 9c282c3 Compare October 7, 2024 15:45

joyeecheung marked this pull request as ready for review October 7, 2024 15:47

joyeecheung force-pushed the backport-require-esm-2 branch from 9c282c3 to 3d0afe0 Compare October 7, 2024 15:52

joyeecheung force-pushed the backport-require-esm-2 branch 2 times, most recently from 5ed6462 to 898396a Compare October 8, 2024 19:00

mcollina approved these changes Oct 9, 2024

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung force-pushed the backport-require-esm-2 branch from 898396a to 5ea60dc Compare October 9, 2024 17:08

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

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

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: #56927
Refs: #55217
Refs: #52697
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>