Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 3 commits into
base: v22.x-staging
Choose a base branch
from
Open

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

wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 1, 2024 •
edited
Loading

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot 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. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Oct 1, 2024
@RedYetiDev RedYetiDev added module Issues and PRs related to the module subsystem. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Oct 1, 2024
@joyeecheung
Copy link
Member Author

CITGM for v22.x-staging: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3481/
CITGM for this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3482/

@RedYetiDev RedYetiDev added the needs-citgm PRs that need a CITGM CI run. label Oct 1, 2024
@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 2, 2024 •
edited
Loading

Some regressions found:

  1. yarn is expecting ERR_REQUIRE_ESM in its tests, probably to check that they didn't do anything to alter Node.js module loading https://github.com/yarnpkg/berry/blob/33f249b6cc06db1803f03eb5f17306a23d11115c/packages/acceptance-tests/pkg-tests-specs/sources/pnp-esm.test.ts#L617
  2. Gulp tests are breaking because they don't expect the experimental warning coming out of Node.js https://github.com/gulpjs/gulp/blob/2fa4981a910d7bdedb758bd09868620c9bb21d54/test/index.test.js#L90 (I am not sure how they load the .mjs configs, though it seems they are also doing a try-catch of require(config) somewhere)
  3. 2 also happened to Jest.
  4. In local testing, I also noticed that the debug package (used by npm, etc.) has a try { require(esm) } catch { /* fallback */ } pattern here, which only gets improvements out of require(esm) support after the unflagging, but then the experimental warning would also break the output.

I think for backporting to v22.x we should probably do some changes:

a. Provides process.features.require_module for feature detection
b. Do not emit the experimental warning if it's coming out of node_modules

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.

@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 2, 2024 •
edited
Loading

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.

@richardlau
Copy link
Member

I think for backporting to v22.x we should probably do some changes:

a. Provides process.features.require_module for feature detection
b. Do not emit the experimental warning if it's coming out of node_modules

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.

These SGTM

@joyeecheung
Copy link
Member Author

Opened #55241 for feature detection.

@joyeecheung
Copy link
Member Author

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.

@targos
Copy link
Member

targos commented Oct 4, 2024

I just updated v22.x-staging so this now has conflicts because of duplicated commits I guess.

@tats-u tats-u mentioned this pull request Oct 4, 2024
Open
@targos targos mentioned this pull request Oct 5, 2024
Merged
This was referenced Oct 5, 2024
Merged
Open
@joyeecheung joyeecheung marked this pull request as ready for review October 7, 2024 15:47
@joyeecheung
Copy link
Member Author

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.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Oct 8, 2024

Can you rebase please?

@joyeecheung joyeecheung force-pushed the backport-require-esm-2 branch 2 times, most recently from 5ed6462 to 898396a Compare October 8, 2024 19:00
mcollina
mcollina approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

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

@joyeecheung joyeecheung mentioned this pull request Oct 10, 2024
Merged
@joyeecheung @aduh95
module: unflag --experimental-require-module
4d67eb9 
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 <[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 @aduh95
module: use kNodeModulesRE to detect node_modules
6f4e4d5 
This is faster and more consistent with other places using the
regular expression to detect node_modules.

PR-URL: nodejs#55243
Backport-PR-URL: nodejs#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: nodejs#52697
@joyeecheung @aduh95
module: do not warn when require(esm) comes from node_modules
71c2d02 
Some packages have been using try-catch to load require(esm)
in environments that are available. In 23, where require(esm)
is unflagged, we emit an experimental warning when require()
is used to load ESM. To backport require(esm) to older LTS
releases, however, this could break existing CLI output.
To reduce the disruption for LTS, on older release lines,
this commit is applied to skip the warning if require(esm)
comes from node_modules. This warning will be eventually
removed when require(esm) becomes stable.

PR-URL: nodejs#55217
Refs: nodejs#52697
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Oct 12, 2024

I would recommend pulling 1435338 as well, although they land cleanly on top of that, tests are not passing:

=== release test-require-node-modules-warning ===  
…
TypeError: isInsideNodeModules is not a function
    at loadESMFromCJS (node:internal/modules/cjs/loader:1389:30)
    at Module._compile (node:internal/modules/cjs/loader:1521:5)
    at Object..js (node:internal/modules/cjs/loader:1676:16)
    at Module.load (node:internal/modules/cjs/loader:1318:32)
    at Function._load (node:internal/modules/cjs/loader:1128:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:219:24)
    at Module.require (node:internal/modules/cjs/loader:1340:12)
    at require (node:internal/modules/helpers:139:16)

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Oct 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joyeecheung @nodejs-github-bot @richardlau @targos @aduh95 @mcollina @RedYetiDev