esm: emit experimental warnings in common place #42314

JakobJingleheimer · 2022-03-12T16:41:20Z

Previously, experimental warnings were scattered (sometimes by me) around the ESM code with inconsistent messaging. This PR consolidates them to ESMLoader instantiation (so subsequent code does not need to consider whether a warning has yet been emitted).

This PR also adds an experimental warning for Network Imports (which was previously not emitted).

nodejs-github-bot · 2022-03-12T16:41:24Z

Review requested:

@nodejs/loaders
@nodejs/modules

lib/internal/modules/esm/resolve.js

test/es-module/test-esm-experimental-warnings.mjs

lib/internal/modules/esm/loader.js

IRL, both internal and external instances of ESMLoader happen. in the test, only 1 external happens and then the test fails.

GeoffreyBooth · 2022-03-28T02:47:57Z

@JakobJingleheimer I just pushed some commits. I got the test to pass by removing the isInternal flag from the ESMLoader constructor. At least in all of the tests, ESMLoader only ever gets initialized once, by Node’s internals, and never again. I’m not sure when it would get initialized a second time—worker threads?—other than in Node’s own tests that explicitly initialize it. Is there a reason you added this flag?

With that removed, and the console.log lines removed, all the tests pass for me. So I guess unless there was some reason we needed isInternal, hopefully the commits I pushed should make this PR mergeable.

nodejs-github-bot · 2022-03-28T04:03:08Z

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

JakobJingleheimer · 2022-03-28T17:16:44Z

@GeoffreyBooth alas, no: the isInternal flag is necessary for the real world. There are 2 instances of ESMLoader:

The 1^st Node.js uses internally to load custom loaders.

The 2^nd is what the custom loaders get shoved into and runs when user-land code runs.

Without the isInternal, IRL, the specifier resolution warning prints twice (the others don't because they're using emitExperimentalWarning(), which de-dupes).

nodejs-github-bot · 2022-03-28T21:19:39Z

Commit Queue failed

- Loading data for nodejs/node/pull/42314
✔  Done loading data for nodejs/node/pull/42314
----------------------------------- PR info ------------------------------------
Title      esm: emit experimental warnings in common place (#42314)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     JakobJingleheimer:esm/consolidate-experimental-warnings-emission-in-common-place -> nodejs:master
Labels     module, process, experimental, esm, author ready, loaders, commit-queue-squash
Commits    8
 - esm: emit experimental warnings in common place
 - fixup: switch test from substring to regex
 - fixup: delint
 - WIP: switch specifier resolution warning to custom message
 - fixup: remove log
 - fixup: remove isInternal flag for ESMLoader
 - fixup: lint
 - fixup: prevent the specifier resolution warning from being printed twice
Committers 1
 - Geoffrey Booth 
PR-URL: https://github.com/nodejs/node/pull/42314
Reviewed-By: Antoine du Hamel 
Reviewed-By: Geoffrey Booth 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42314
Reviewed-By: Antoine du Hamel 
Reviewed-By: Geoffrey Booth 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup: prevent the specifier resolution warning from being printed twice
   ℹ  This PR was created on Sat, 12 Mar 2022 16:41:20 GMT
   ✔  Approvals: 2
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42314#pullrequestreview-908174329
   ✔  - Geoffrey Booth (@GeoffreyBooth): https://github.com/nodejs/node/pull/42314#pullrequestreview-923226427
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-03-28T18:10:29Z: https://ci.nodejs.org/job/node-test-pull-request/43219/
- Querying data for job/node-test-pull-request/43219/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2054946216

nodejs-github-bot · 2022-03-28T21:43:17Z

Landed in 5a927ef

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

PR-URL: nodejs#42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

kachkaev · 2022-04-24T13:58:47Z

👋 @JakobJingleheimer! I’m not familiar with Node.js internals, but I guess that this change could potentially break a workaround in here: #30810 Could you please share your thoughts in that issue if it’s relevant?

~~It’d be great to know how to suppress warnings in Node 18 like in Node 16.~~
Found a working patch: #30810 (comment)

PR-URL: nodejs#42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

PR-URL: nodejs/node#42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

JakobJingleheimer added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Mar 12, 2022

JakobJingleheimer requested a review from GeoffreyBooth March 12, 2022 16:41

nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Mar 12, 2022

GeoffreyBooth reviewed Mar 12, 2022

View reviewed changes

lib/internal/modules/esm/resolve.js Show resolved Hide resolved

aduh95 reviewed Mar 12, 2022

View reviewed changes

test/es-module/test-esm-experimental-warnings.mjs Outdated Show resolved Hide resolved

aduh95 reviewed Mar 12, 2022

View reviewed changes

test/es-module/test-esm-experimental-warnings.mjs Outdated Show resolved Hide resolved

test/es-module/test-esm-experimental-warnings.mjs Outdated Show resolved Hide resolved

JakobJingleheimer force-pushed the esm/consolidate-experimental-warnings-emission-in-common-place branch from 39bf611 to bfe82dc Compare March 13, 2022 16:00

aduh95 approved these changes Mar 13, 2022

View reviewed changes

jasnell reviewed Mar 25, 2022

View reviewed changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

JakobJingleheimer added 4 commits March 27, 2022 18:34

esm: emit experimental warnings in common place

49558a1

fixup: switch test from substring to regex

61c0135

fixup: delint

463ae28

WIP: switch specifier resolution warning to custom message

59eff1f

IRL, both internal and external instances of ESMLoader happen. in the test, only 1 external happens and then the test fails.

GeoffreyBooth force-pushed the esm/consolidate-experimental-warnings-emission-in-common-place branch from cc6202a to 59eff1f Compare March 28, 2022 01:35

GeoffreyBooth added 2 commits March 27, 2022 19:14

fixup: remove log

ad57f42

fixup: remove isInternal flag for ESMLoader

592bf88

This comment was marked as outdated.

Sign in to view

GeoffreyBooth added module Issues and PRs related to the module subsystem. experimental Issues and PRs related to experimental features. and removed needs-ci PRs that need a full CI run. labels Mar 28, 2022

fixup: lint

35bb8b0

This comment was marked as outdated.

Sign in to view

GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 28, 2022

GeoffreyBooth approved these changes Mar 28, 2022

View reviewed changes

GeoffreyBooth added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 28, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 28, 2022

GeoffreyBooth approved these changes Mar 28, 2022

View reviewed changes

GeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 28, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 28, 2022

nodejs-github-bot merged commit 5a927ef into nodejs:master Mar 28, 2022

GeoffreyBooth deleted the esm/consolidate-experimental-warnings-emission-in-common-place branch March 28, 2022 21:57

juanarbol pushed a commit that referenced this pull request Apr 4, 2022

esm: emit experimental warnings in common place

ea5ea1b

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022

esm: emit experimental warnings in common place

eea25dc

PR-URL: nodejs#42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

This was referenced Apr 5, 2022

V17.8.1 proposal #42612

Closed

v17.9.0 proposal #42613

Merged

juanarbol pushed a commit that referenced this pull request Apr 6, 2022

esm: emit experimental warnings in common place

902776e

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

merceyz mentioned this pull request Apr 9, 2022

fix(pnp): suppress experimental ESM loader warning yarnpkg/berry#4338

Merged

3 tasks

GeoffreyBooth mentioned this pull request Apr 11, 2022

esm: add chaining to loaders #42623

Merged

10 tasks

JakobJingleheimer mentioned this pull request Apr 24, 2022

Ability to suppress warnings by type (or just experimental warnings) #30810

Closed

xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022

esm: emit experimental warnings in common place

a479baa

PR-URL: nodejs#42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

juanarbol pushed a commit that referenced this pull request May 31, 2022

esm: emit experimental warnings in common place

89eac97

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

danielleadams pushed a commit that referenced this pull request Jun 27, 2022

esm: emit experimental warnings in common place

62b59df

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

targos pushed a commit that referenced this pull request Jul 11, 2022

esm: emit experimental warnings in common place

1af7143

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

targos pushed a commit that referenced this pull request Jul 11, 2022

esm: emit experimental warnings in common place

20a3954

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

targos pushed a commit that referenced this pull request Jul 31, 2022

esm: emit experimental warnings in common place

14a929b

PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: emit experimental warnings in common place #42314

esm: emit experimental warnings in common place #42314

JakobJingleheimer commented Mar 12, 2022 •

edited

Loading

nodejs-github-bot commented Mar 12, 2022 •

edited by JakobJingleheimer

Loading

GeoffreyBooth commented Mar 28, 2022

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Mar 28, 2022

JakobJingleheimer commented Mar 28, 2022 •

edited

Loading

nodejs-github-bot commented Mar 28, 2022

nodejs-github-bot commented Mar 28, 2022

kachkaev commented Apr 24, 2022 •

edited

Loading

esm: emit experimental warnings in common place #42314

esm: emit experimental warnings in common place #42314

Conversation

JakobJingleheimer commented Mar 12, 2022 • edited Loading

nodejs-github-bot commented Mar 12, 2022 • edited by JakobJingleheimer Loading

GeoffreyBooth commented Mar 28, 2022

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Mar 28, 2022

JakobJingleheimer commented Mar 28, 2022 • edited Loading

nodejs-github-bot commented Mar 28, 2022

nodejs-github-bot commented Mar 28, 2022

kachkaev commented Apr 24, 2022 • edited Loading

JakobJingleheimer commented Mar 12, 2022 •

edited

Loading

nodejs-github-bot commented Mar 12, 2022 •

edited by JakobJingleheimer

Loading

JakobJingleheimer commented Mar 28, 2022 •

edited

Loading

kachkaev commented Apr 24, 2022 •

edited

Loading