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

esm: use "node:" internal namespace for builtins #35387

Closed
wants to merge 5 commits into from
Closed

esm: use "node:" internal namespace for builtins #35387

wants to merge 5 commits into from

Conversation

guybedford
Copy link
Contributor

This has been discussed a couple of times in the past few meetings that the internal representation for builtins should align with policies using the node: scheme as well as to allow this to be the scheme to make public for users in future if we ever want to expose it more explicitly in any modules APIs or interfaces.

This is strictly a minor breaking change, but should have relatively low usage in the wild and is entirely limited to the experimental modules and loader cases.

@nodejs/modules-active-members

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 28, 2020
@guybedford guybedford changed the title esm: use "node:" namespace for builtins esm: use "node:" internal namespace for builtins Sep 28, 2020
@devsnek
Copy link
Member

devsnek commented Sep 28, 2020

To be very explicit, this breaks any code which imports nodejs:xyz.

@ljharb
Copy link
Member

ljharb commented Sep 28, 2020

The likelihood that there's lots of code that uses a protocol whatsoever is vanishingly small; it seems like a good idea to get this change into v15.

bmeck
bmeck approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

👍

aduh95
aduh95 reviewed Sep 28, 2020
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Maybe we could use primordials here?

lib/internal/modules/esm/translators.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/translators.js Outdated Show resolved Hide resolved
mcollina
mcollina approved these changes Sep 28, 2020
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/33283/

@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 29, 2020
guybedford added a commit that referenced this pull request Sep 30, 2020
@guybedford
esm: use "node:" namespace for builtins
ee9e3e7 
PR-URL: #35387
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@guybedford
Copy link
Contributor Author

Landed in ee9e3e7.

@guybedford guybedford closed this Sep 30, 2020
@bmeck bmeck mentioned this pull request Sep 30, 2020
Closed
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
@guybedford @danielleadams
esm: use "node:" namespace for builtins
91b820e 
PR-URL: #35387
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
Merged
@aduh95 aduh95 mentioned this pull request Oct 23, 2020
Closed
3 tasks
aduh95 pushed a commit to aduh95/node that referenced this pull request Oct 27, 2020
@guybedford @aduh95
esm: use "node:" namespace for builtins
3e18962 
PR-URL: nodejs#35387
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@ExE-Boss ExE-Boss mentioned this pull request Oct 29, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
@guybedford @MylesBorins
esm: use "node:" namespace for builtins
9099190 
Backport-PR-URL: #35757
PR-URL: #35387
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@guybedford @MylesBorins
esm: use "node:" namespace for builtins
0f757bc 
Backport-PR-URL: #35757
PR-URL: #35387
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@guybedford @joesepi
esm: use "node:" namespace for builtins
2a9e6b6 
PR-URL: nodejs#35387
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
kevinoid added a commit to kevinoid/node-project-template that referenced this pull request May 6, 2021
@kevinoid
Require Node.js >= 12.20, >= 14.13.1
c3259a2 
Which added support for named exports for CJS via static analysis:
nodejs/node#35249
nodejs/node@1e8cb08edc for v15
nodejs/node@f551f52f83 for v14.13
nodejs/node@9eb1fa1924 for v12.20

This is also necessary for `node:` scheme support, checked by the
`unicorn/prefer-node-protocol` ESLint rule (currently disabled pending
support in eslint-plugin-node):
nodejs/node#35387
nodejs/node@ee9e3e75aa for v15
nodejs/node@91b820e939 for v14.13.1
nodejs/node@0f757bc2df for v12.20

Signed-off-by: Kevin Locke <[email protected]>
kevinoid added a commit to kevinoid/hub-ci-status that referenced this pull request Aug 5, 2021
@kevinoid
Require Node.js >= 12.20, >= 14.13.1
11f7be9 
Which added support for named exports for CJS via static analysis:
nodejs/node#35249
nodejs/node@1e8cb08edc for v15
nodejs/node@f551f52f83 for v14.13
nodejs/node@9eb1fa1924 for v12.20

This is also necessary for `node:` scheme support, checked by the
`unicorn/prefer-node-protocol` ESLint rule (currently disabled pending
support in eslint-plugin-node):
nodejs/node#35387
nodejs/node@ee9e3e75aa for v15
nodejs/node@91b820e939 for v14.13.1
nodejs/node@0f757bc2df for v12.20

Signed-off-by: Kevin Locke <[email protected]>
@mattcollier mattcollier mentioned this pull request Jan 25, 2022
Closed
DeeDeeG added a commit to DeeDeeG/node that referenced this pull request Jul 11, 2022
@DeeDeeG
doc: revert anachronistic 'node:' module require()s in API history notes
c3459bb 
Support for the 'node:' prefixed builtin module namespace was introduced
for `require()` expressions in Node v16.0.0, and backported to v14.18.0.
This was never supported in Node v15.x or chronologically older.

All of the current API history notes in the docs using 'node:' prefixed
module `require()`s happen to be documenting changes in Node versions
from before the time when support was first introduced.

This commit reverts those `require()`s in the history notes to be
un-prefixed. (They were incorrect as written; The prefixed `require()`s
would not work for those older Node versions.)

This change prevents the API history notes from inaccurately implying
'node:' prefixed builtin modules were introduced many Node versions ago,
or were `require()`-able with the 'node:' prefix in those Node versions.

Refs: nodejs#35387
Refs: nodejs#37246
Refs: nodejs#42752
@DeeDeeG DeeDeeG mentioned this pull request Jul 11, 2022
Merged
nodejs-github-bot pushed a commit that referenced this pull request Jul 14, 2022
@DeeDeeG
doc: revert anachronistic 'node:' module require()s in API history notes
48d4e3d 
Support for the 'node:' prefixed builtin module namespace was introduced
for `require()` expressions in Node v16.0.0, and backported to v14.18.0.
This was never supported in Node v15.x or chronologically older.

All of the current API history notes in the docs using 'node:' prefixed
module `require()`s happen to be documenting changes in Node versions
from before the time when support was first introduced.

This commit reverts those `require()`s in the history notes to be
un-prefixed. (They were incorrect as written; The prefixed `require()`s
would not work for those older Node versions.)

This change prevents the API history notes from inaccurately implying
'node:' prefixed builtin modules were introduced many Node versions ago,
or were `require()`-able with the 'node:' prefix in those Node versions.

Refs: #35387
Refs: #37246
Refs: #42752

PR-URL: #43768
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 26, 2022
@DeeDeeG @danielleadams
doc: revert anachronistic 'node:' module require()s in API history notes
9b53a69 
Support for the 'node:' prefixed builtin module namespace was introduced
for `require()` expressions in Node v16.0.0, and backported to v14.18.0.
This was never supported in Node v15.x or chronologically older.

All of the current API history notes in the docs using 'node:' prefixed
module `require()`s happen to be documenting changes in Node versions
from before the time when support was first introduced.

This commit reverts those `require()`s in the history notes to be
un-prefixed. (They were incorrect as written; The prefixed `require()`s
would not work for those older Node versions.)

This change prevents the API history notes from inaccurately implying
'node:' prefixed builtin modules were introduced many Node versions ago,
or were `require()`-able with the 'node:' prefix in those Node versions.

Refs: #35387
Refs: #37246
Refs: #42752

PR-URL: #43768
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@DeeDeeG @targos
doc: revert anachronistic 'node:' module require()s in API history notes
44291af 
Support for the 'node:' prefixed builtin module namespace was introduced
for `require()` expressions in Node v16.0.0, and backported to v14.18.0.
This was never supported in Node v15.x or chronologically older.

All of the current API history notes in the docs using 'node:' prefixed
module `require()`s happen to be documenting changes in Node versions
from before the time when support was first introduced.

This commit reverts those `require()`s in the history notes to be
un-prefixed. (They were incorrect as written; The prefixed `require()`s
would not work for those older Node versions.)

This change prevents the API history notes from inaccurately implying
'node:' prefixed builtin modules were introduced many Node versions ago,
or were `require()`-able with the 'node:' prefix in those Node versions.

Refs: #35387
Refs: #37246
Refs: #42752

PR-URL: #43768
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@DeeDeeG @guangwong
doc: revert anachronistic 'node:' module require()s in API history notes
7eab26b 
Support for the 'node:' prefixed builtin module namespace was introduced
for `require()` expressions in Node v16.0.0, and backported to v14.18.0.
This was never supported in Node v15.x or chronologically older.

All of the current API history notes in the docs using 'node:' prefixed
module `require()`s happen to be documenting changes in Node versions
from before the time when support was first introduced.

This commit reverts those `require()`s in the history notes to be
un-prefixed. (They were incorrect as written; The prefixed `require()`s
would not work for those older Node versions.)

This change prevents the API history notes from inaccurately implying
'node:' prefixed builtin modules were introduced many Node versions ago,
or were `require()`-able with the 'node:' prefix in those Node versions.

Refs: nodejs/node#35387
Refs: nodejs/node#37246
Refs: nodejs/node#42752

PR-URL: nodejs/node#43768
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@targos targos targos approved these changes

@jkrems jkrems jkrems approved these changes

@bmeck bmeck bmeck 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. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@guybedford @nodejs-github-bot @devsnek @ljharb @mcollina @bmeck @jkrems @targos @aduh95