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

module: only emit require(esm) warning under --trace-require-module #56194

Closed
wants to merge 2 commits into from
Closed

module: only emit require(esm) warning under --trace-require-module #56194

wants to merge 2 commits into from
+80 −48

Conversation

joyeecheung
Copy link
Member

require(esm) is relatively stable now and the experimental warning has run its course - it's now more troublesome than useful. This patch changes it to no longer emit a warning unless --trace-require-module is explicitly used. The flag supports two modes:

  • --trace-require-module=all: emit warnings for all usages
  • --trace-require-module=no-node-modules: emit warnings for usages that do not come from a node_modules folder.

Fixes: #55417
Refs: #55085 (comment)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Dec 9, 2024
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2024
@joyeecheung
module: only emit require(esm) warning under --trace-require-module
4dde51d 
require(esm) is relatively stable now and the experimental warning
has run its course - it's now more troublesome than useful.
This patch changes it to no longer emit a warning unless
`--trace-require-module` is explicitly used. The flag supports
two modes:

- `--trace-require-module=all`: emit warnings for all usages
- `--trace-require-module=no-node-modules`: emit warnings for
  usages that do not come from a `node_modules` folder.
jasnell
jasnell reviewed Dec 9, 2024
View reviewed changes
doc/api/modules.md Outdated Show resolved Hide resolved
@joyeecheung joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Dec 9, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2024
@nodejs-github-bot
Copy link
Collaborator

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

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

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

@codecov Codecov
Copy link

codecov bot commented Dec 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.49%. Comparing base (b915124) to head (9351661).
Report is 63 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #56194      +/-   ##
==========================================
+ Coverage   87.99%   88.49%   +0.50%     
==========================================
  Files         656      656              
  Lines      188959   189271     +312     
  Branches    35979    36346     +367     
==========================================
+ Hits       166267   167496    +1229     
+ Misses      15867    14973     -894     
+ Partials     6825     6802      -23
Files with missing lines Coverage Δ
lib/internal/modules/cjs/loader.js 98.30% <100.00%> (+3.99%) ⬆️
src/node_options.cc 87.96% <100.00%> (+0.08%) ⬆️
src/node_options.h 98.29% <ø> (ø)

... and 101 files with indirect coverage changes

@aduh95 aduh95 added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 10, 2024
@joyeecheung
Copy link
Member Author

I don't know if it's necessary since we aren't changing the stability index (I was thinking about changing it and then realized that it was already release candidate), but anyway cc @nodejs/tsc for awareness

@joyeecheung joyeecheung added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @joyeecheung.

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.

mcollina
mcollina approved these changes Dec 10, 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

The warning actually broke one of my tests in v22 :).

@joyeecheung joyeecheung added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 11, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 11, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 586814b...b6c9dbe

nodejs-github-bot pushed a commit that referenced this pull request Dec 11, 2024
@joyeecheung @nodejs-github-bot
module: only emit require(esm) warning under --trace-require-module
b6c9dbe 
require(esm) is relatively stable now and the experimental warning
has run its course - it's now more troublesome than useful.
This patch changes it to no longer emit a warning unless
`--trace-require-module` is explicitly used. The flag supports
two modes:

- `--trace-require-module=all`: emit warnings for all usages
- `--trace-require-module=no-node-modules`: emit warnings for
  usages that do not come from a `node_modules` folder.

PR-URL: #56194
Fixes: #55417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@joyeecheung joyeecheung mentioned this pull request Dec 12, 2024
Open
9 tasks
targos pushed a commit that referenced this pull request Dec 13, 2024
@joyeecheung @targos
module: only emit require(esm) warning under --trace-require-module
9863d27 
require(esm) is relatively stable now and the experimental warning
has run its course - it's now more troublesome than useful.
This patch changes it to no longer emit a warning unless
`--trace-require-module` is explicitly used. The flag supports
two modes:

- `--trace-require-module=all`: emit warnings for all usages
- `--trace-require-module=no-node-modules`: emit warnings for
  usages that do not come from a `node_modules` folder.

PR-URL: #56194
Fixes: #55417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@github-actions github-actions bot mentioned this pull request Dec 18, 2024
Merged
nodejs-github-bot added a commit that referenced this pull request Dec 18, 2024
@nodejs-github-bot
2024-12-19, Version 23.5.0 (Current)
7064e69 
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201
stream:
  * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096

PR-URL: #56310
aduh95 added a commit that referenced this pull request Dec 18, 2024
@aduh95
2024-12-19, Version 23.5.0 (Current)
317575a 
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201
stream:
  * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096

PR-URL: TODO
aduh95 pushed a commit that referenced this pull request Dec 18, 2024
@nodejs-github-bot @aduh95
2024-12-19, Version 23.5.0 (Current)
d1b4286 
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201
stream:
  * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096

PR-URL: #56310
aduh95 added a commit that referenced this pull request Dec 18, 2024
@aduh95
2024-12-19, Version 23.5.0 (Current)
d8c3089 
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
dgram:
  * (SEMVER-MINOR) support blocklist in udp (theanarkh) #56087
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201

PR-URL: #56310
aduh95 added a commit that referenced this pull request Dec 19, 2024
@aduh95
2024-12-19, Version 23.5.0 (Current)
854d23b 
Notable changes:

crypto:
  * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142
dgram:
  * (SEMVER-MINOR) support blocklist in udp (theanarkh) #56087
doc:
  * stabilize util.styleText (Rafael Gonzaga) #56265
module:
  * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185
  * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194
  * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698
  * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698
report:
  * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068
sqlite:
  * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213
src,lib:
  * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201

PR-URL: #56310
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@aduh95 aduh95 aduh95 approved these changes

@anonrig anonrig anonrig approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can we supress the new experimental warning from the new default require(esm) setting?
9 participants
@joyeecheung @nodejs-github-bot @mcollina @jasnell @GeoffreyBooth @anonrig @aduh95 @RafaelGSS @marco-ippolito