esm: emit experimental warnings in common place #42314
Merged
+56
−35
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JakobJingleheimer
added
esm
|
Review requested:
|
nodejs-github-bot
added
needs-ci
aduh95
reviewed
Mar 12, 2022
aduh95
reviewed
Mar 12, 2022
JakobJingleheimer
force-pushed
the
esm/consolidate-experimental-warnings-emission-in-common-place
branch
from
39bf611
to
bfe82dc
Compare
aduh95
approved these changes
Mar 13, 2022
jasnell
reviewed
Mar 25, 2022
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
|
@JakobJingleheimer I just pushed some commits. I got the test to pass by removing the With that removed, and the |
This comment was marked as outdated.
This comment was marked as outdated.
GeoffreyBooth
added
module
This comment was marked as outdated.
This comment was marked as outdated.
GeoffreyBooth
added
the
author ready
GeoffreyBooth
approved these changes
Mar 28, 2022
|
@GeoffreyBooth alas, no: the The 1st Node.js uses internally to load custom loaders. The 2nd is what the custom loaders get shoved into and runs when user-land code runs. Without the |
GeoffreyBooth
added
commit-queue-squash
nodejs-github-bot
added
commit-queue-failed
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/.ncuhttps://github.com/nodejs/node/actions/runs/2054946216 |
GeoffreyBooth
approved these changes
Mar 28, 2022
GeoffreyBooth
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 5a927ef |
GeoffreyBooth
deleted the
esm/consolidate-experimental-warnings-emission-in-common-place
branch
juanarbol
pushed a commit
that referenced
this pull request
Apr 4, 2022
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
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
Apr 6, 2022
PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
3 tasks
10 tasks
|
👋 @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?
|
xtx1130
pushed a commit
to xtx1130/node
that referenced
this pull request
Apr 25, 2022
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
PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams
pushed a commit
that referenced
this pull request
Jun 27, 2022
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
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
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
PR-URL: #42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
guangwong
pushed a commit
to noslate-project/node
that referenced
this pull request
Oct 10, 2022
PR-URL: nodejs/node#42314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).