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.

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

Use the node: protocol in docs examples #38343

Closed
sindresorhus opened this issue Apr 22, 2021 · 20 comments · Fixed by #42752
Closed

Use the node: protocol in docs examples #38343

sindresorhus opened this issue Apr 22, 2021 · 20 comments · Fixed by #42752
Labels
doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.

Comments

@sindresorhus
Copy link

sindresorhus commented Apr 22, 2021

The node: protocol has many benefits:

  • Makes it perfectly clear that the import is a Node.js builtin module. (Beginners don't always realize this)
  • Makes the import identifier a valid absolute URL.
  • Avoids conflicts for future Node.js builtin modules.

However, most people don't know about the node: protocol. I suggest Node.js makes it the recommended way of importing builtin modules by using it in all the examples in the documentation. A lot of people are starting to move to ESM, so now would be a good time for people to switch to the node: protocol too.

Personally, I plan to switch all my packages to use the node: protocol. We have also made an ESLint rule for it.

@sindresorhus sindresorhus added the doc Issues and PRs related to the documentations. label Apr 22, 2021
@aduh95
Copy link
Contributor

aduh95 commented Apr 22, 2021

We may want to wait for this support for node: on require() calls to be backported to v14.x before changing all the docs, but I'm all for it personally.

We have also made an ESLint rule for it.

That's really nice, thanks for doing this!

@SimenB
Copy link
Member

SimenB commented Apr 22, 2021

Will require('module').builtinModules include them?

@aduh95
Copy link
Contributor

aduh95 commented Apr 22, 2021

Will require('module').builtinModules include them?

Currently, it doesn't. In ESM world, a specifier using the node: protocol can only reference a built-in core module. In CJS, as of Node.js v16.0.0, a module whose name starts with node: can also only reference a built-in core module. Given that, would it be useful for require('node:module').builtinModules to include both prefixed and non-prefixed versions?

@SimenB
Copy link
Member

SimenB commented Apr 22, 2021

Not 100% sure, but as the docs state

Can be used to verify if a module is maintained by a third party or not.

If I also have to inspect what the identifier is and change that string, isn't it strictly less useful for that use case? "Use this function to check if modules i first party, oh and btw you also need to strip node: prefix if it exists", sorta?

Or do you mean "in addition to this list, all modules prefixed with node: should be considered core" and that list is for all non-prefixed modules? Will any modules prefixed with node: ever not also be available without the prefix (e.g. a future node:http3 and http)?

(this is getting off topic, might be better to open a new issue)

EDIT: I guess this is an issue for fs/promise etc as well...

@aduh95
Copy link
Contributor

aduh95 commented Apr 22, 2021

Or do you mean "in addition to this list, all modules prefixed with node: should be considered core" and that list is for all non-prefixed modules?

I think that's correct, if a user tries to require("node:custom"), it throws a ERR_UNKNOWN_BUILTIN_MODULE error.

Will any modules prefixed with node: ever not also be available without the prefix (e.g. a future node:http3 and http)?

So far that has never been the case, but could change in the future. Currently, the introduction of a new core module (E.G.: readline/promises) is a breaking change, as it might clash with a user module of the same name. We might decide to introduce new modules in a non-breaking way by allowing it only with node: prefix. It's still very hypothetical though.

(this is getting off topic, might be better to open a new issue)

(I agree 😅)

I guess this is an issue for fs/promise etc as well...

Note sure what you mean by that, fs/promises is listed in the builtinModules array, I don't think it is any different from any other core module.

@kuzmaMinin

This comment has been minimized.

@kuzmaMinin

This comment has been minimized.

@kuzmaMinin

This comment has been minimized.

@kuzmaMinin

This comment has been minimized.

@jasnell
Copy link
Member

jasnell commented Apr 22, 2021

We may want to wait for this support for node: on require() calls to be backported to v14.x before changing all the docs, but I'm all for it personally.

I think it makes sense to go ahead and do in docs in the main branch. It could take some time before things get backported to v14.x.

@aduh95 aduh95 added the good first issue Issues that are suitable for first-time contributors. label Apr 22, 2021
@guybedford
Copy link
Contributor

guybedford commented Apr 22, 2021

My one major concern about this pattern becoming engrained is it means truly locking down that import maps should map all URLs and not just bare specifiers. Eg a package on unpkg that wants to shim a Node.js builtin with an import map. From a sort of security perspective I still like the concept of import maps only working on bare specifiers as it makes the security holes in the module graph absolutely clear from a resolution perspective, while this ship has most likely sailed to bring in this higher order concern here, it still seems a slight risk to me.

The concern being that if import maps were to change to disabling absolute URL mappings at some point it might present an issue to mapping builtins for existing modules containing import 'node:fs'.

@NickRimer03

This comment has been minimized.

@jeromecovington
Copy link
Contributor

@sindresorhus @jasnell et al. I've opened PR #38620 addressing this change in all the api docs. Please let me know what you think when you have time.

@ljharb
Copy link
Member

ljharb commented May 10, 2021

I don't think it's a good idea to use node: in docs until all LTS versions support it in CJS and ESM.

@Trott
Copy link
Member

Trott commented May 12, 2021

I don't think it's a good idea to use node: in docs until all LTS versions support it in CJS and ESM.

@ljharb I might be stating the obvious here, but the docs are for specific versions, so we could update it for the versions that support it, and leave it as is for the versions that don't.

@ljharb
Copy link
Member

ljharb commented May 12, 2021

@Trott while that is totally true, in my experience, most people have no idea that the default docs they land on are for latest node, even if they're not using latest node.

@theoludwig
Copy link
Contributor

@Trott while that is totally true, in my experience, most people have no idea that the default docs they land on are for latest node, even if they're not using latest node.

That's their problem I guess, they should read the documentation appropriate to their Node version, if they don't they will take responsibility to try something from the latest documentation that don't work for their version.

Each Node.js version has its documentation so IMO there is really no problem for that change!

@ljharb
Copy link
Member

ljharb commented May 13, 2021

While that’s also true, the failure for downstream package consumers might be a bit more complex to debug with a node:-prefixed specifier than with a nonexistent feature. Things that affect published package consumers are always a lot more complex than things that affect local codebases.

aduh95 added a commit that referenced this issue Apr 20, 2022
Some core modules can be loaded with or without the `node:` prefix.
Using the prefix disambiguates which specifiers refer to core modules.

This commit updates the docs to use the prefix everywhere a core module
is referenced.

PR-URL: #42752
Fixes: #38343
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
Some core modules can be loaded with or without the `node:` prefix.
Using the prefix disambiguates which specifiers refer to core modules.

This commit updates the docs to use the prefix everywhere a core module
is referenced.

PR-URL: nodejs#42752
Fixes: nodejs#38343
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
targos pushed a commit that referenced this issue Apr 28, 2022
Some core modules can be loaded with or without the `node:` prefix.
Using the prefix disambiguates which specifiers refer to core modules.

This commit updates the docs to use the prefix everywhere a core module
is referenced.

PR-URL: #42752
Fixes: #38343
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
juanarbol pushed a commit that referenced this issue May 31, 2022
Some core modules can be loaded with or without the `node:` prefix.
Using the prefix disambiguates which specifiers refer to core modules.

This commit updates the docs to use the prefix everywhere a core module
is referenced.

PR-URL: #42752
Fixes: #38343
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
danielleadams pushed a commit that referenced this issue Jun 27, 2022
Some core modules can be loaded with or without the `node:` prefix.
Using the prefix disambiguates which specifiers refer to core modules.

This commit updates the docs to use the prefix everywhere a core module
is referenced.

PR-URL: #42752
Fixes: #38343
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
targos pushed a commit that referenced this issue Jul 12, 2022
Some core modules can be loaded with or without the `node:` prefix.
Using the prefix disambiguates which specifiers refer to core modules.

This commit updates the docs to use the prefix everywhere a core module
is referenced.

PR-URL: #42752
Fixes: #38343
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
targos pushed a commit that referenced this issue Jul 31, 2022
Some core modules can be loaded with or without the `node:` prefix.
Using the prefix disambiguates which specifiers refer to core modules.

This commit updates the docs to use the prefix everywhere a core module
is referenced.

PR-URL: #42752
Fixes: #38343
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@pauldraper
Copy link

pauldraper commented Aug 16, 2022

My one major concern about this pattern becoming engrained is it means truly locking down that import maps should map all URLs and not just bare specifiers. Eg a package on unpkg that wants to shim a Node.js builtin with an import map. From a sort of security perspective I still like the concept of import maps only working on bare specifiers as it makes the security holes in the module graph absolutely clear from a resolution perspective

If there is any reason to "lock down" module resolutions, would not the Node.js built-ins be of the absolute highest priority to lock down?

I agree that it can make sense -- even then -- to tamper with the resolution.....but that's a statement about the high utility in tweaking resolution in general.

tl;dr Import maps should work for all specifiers.

@aduh95
Copy link
Contributor

aduh95 commented Aug 16, 2022

@pauldraper node:fs is a valid absolute URL, it was unclear at the time of the original comment if import maps would be allowed to modify absolute URL (e.g. take import "http://example.com/" and the import map {"imports": {"http://example.com/":"http://evil-domain.com/"}}, should if load from example.com or evil-domain.com?). I'm not sure this was ever clarified, but at least Chrome implementation seems to accept "rewriting" absolute URLs (i.e. it would load from evil-domain.com), and Node.js moved forward with this change in our docs.

guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
Some core modules can be loaded with or without the `node:` prefix.
Using the prefix disambiguates which specifiers refer to core modules.

This commit updates the docs to use the prefix everywhere a core module
is referenced.

PR-URL: nodejs/node#42752
Fixes: nodejs/node#38343
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
undergroundwires added a commit to undergroundwires/privacy.sexy that referenced this issue Dec 28, 2023
- Bump Node.js to version 18. This change is necessary as Node.js v16
  will reach end-of-life on 2023-09-11. It also ensure compatibility
  with dependencies requiring minimum of Node.js v18, such as `vite`,
  `@vitejs`plugin-legacy` and `icon-gen`.
- Bump `setup-node` action to v4.
- Recommend using the `nvm` tool for managing Node.js versions in the
  documentation.
- Update documentation to point to code reference for required Node.js
  version. This removes duplication of information, and keeps the code
  as single source of truth for required Node.js version.
- Refactor code to adopt the `node:` protocol for Node API imports as
  per Node.js 18 standards. This change addresses ambiguities and aligns
  with Node.js best practices (nodejs/node#38343). Currently, there is
  no ESLint rule to enforce this protocol, as noted in
  import-js/eslint-plugin-import#2717.
- Replace `cross-fetch` dependency with the native Node.js fetch API
  introduced in Node.js 18. Adjust type casting for async iterable read
  streams to align with the latest Node.js APIs, based on discussions in
  DefinitelyTyped/DefinitelyTyped#65542.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.