doc: add node protocol to ESM and CommonJS api examples #38620

jeromecovington · 2021-05-10T13:21:20Z

This PR addresses #38343. As far as I can tell that issue may have only been concerned with updating the node: protocol for ESM examples, but I think it makes sense to update for CommonJS examples as well, as it seems to me that it also works for that. I'm not sure if there are other areas of the docs other than the api examples that need this change, that's all I've looked at so far.

jeromecovington · 2021-05-10T14:54:22Z

I'll make sure to clean up those markdown lint issues.

Trott

LGTM although depending on collaborator consensus, landing may have to wait.

Trott · 2021-05-16T22:42:59Z

@nodejs/documentation

DerekNonGeneric

These all look great, thanks.

DerekNonGeneric · 2021-05-17T00:16:36Z

Well, as @ljharb said, wait until node protocol is backported to LTS. I had not seen that comment yet.

Trott · 2021-05-17T02:36:02Z

Well, as @ljharb said, wait until node protocol is backported to LTS. I had not seen that comment yet.

Are there plans to backport node: to 14.x?

DerekNonGeneric · 2021-05-17T04:13:53Z

@Trott, core modules should be documented this way. I would say to give it some time for people to gain awareness. I am not aware of any other prior discussions about it though.

DerekNonGeneric

@jeromecovington, here is a second-pass review. I will have to get back to you once I have the answer to this, but perhaps @GeoffreyBooth knows what should be stated in these code blocks?

doc/api/esm.md

DerekNonGeneric · 2021-05-17T06:07:48Z

doc/api/esm.md

@@ -340,11 +340,11 @@ module default import or its corresponding sugar syntax:

 <!-- eslint-disable no-duplicate-imports -->
 ```js
-import { default as cjs } from 'cjs';
+import { default as cjs } from 'node:cjs';


This seems wrong to me after looking at it again. There is no core module named cjs.

Thanks I'll be sure to fix this.

@DerekNonGeneric I fixed this, and a couple other instances of node:cjs.

/cc @nodejs/modules if anyone knows why the code blocks here have a cjs specifier

doc/api/esm.md

targos · 2021-05-17T08:46:28Z

Well, as @ljharb said, wait until node protocol is backported to LTS. I had not seen that comment yet.

Are there plans to backport node: to 14.x?

~~No. It was a semver-major change: #35498~~

Edit: sorry, that was another change...

Trott · 2021-05-19T04:24:16Z

@jeromecovington I don't know how this pull request is going to play out, but thank you for your patience and diligence on this, whatever happens!

Requested changes have been addressed.

Trott · 2021-05-23T23:34:20Z

@jasnell You seemed to express support for something like this in #38343 (comment). What do you think of this PR?

Trott · 2021-05-23T23:35:51Z

@guybedford Given #38343 (comment), I'm guessing there's nothing that can be done in this PR to address your reservation expressed there. But if I'm wrong, please do indicate what might happen to make this PR something you endorse, or at least don't have any significant reservations about.

jasnell · 2021-05-23T23:38:12Z

I'm in favor of the change but there's no particular rush on it

Trott · 2021-12-17T04:25:04Z

@jeromecovington Thanks for the email ping. If you're up for rebasing to address all the conflicts, this may be ready to go.

@guybedford Is node: well-established enough now that you don't have concerns about documenting it widely like this?

@aduh95 node: is available on 14.x so I imagine the backport-blocked-v14.x can be removed. Is that right?

guybedford · 2021-12-17T21:59:50Z

@Trott yes I wouldn't block this PR anymore personally.

ljharb · 2021-12-17T22:07:54Z

fwiw use of node: in a package still breaks consumers in surprising ways based on reports across a number of my packages; i'd still prefer node stick with the bare form in the docs.

aduh95 · 2021-12-17T22:12:25Z

@ljharb do you have examples of such reports? How is it breaking? I think we could reach out to the maintainers of the tools that don't handle those correctly to gather more feedback on this.

ljharb · 2021-12-17T22:21:49Z

@aduh95 one reason is that the resolve module only understands core modules that the current version of node does. This means if someone's using node 16 in production, but their IDE or editor uses node 12 (which either doesn't, or didn't until recently, support node: in require), they'd get linter warnings despite things working fine in production. There's no good fix here since, eg, vscode doesn't make it easy to change which node version is bundled with it.

jasnell

I'm generally +1 on this change but I think we should wait until 12.x reaches end of LTS support in April 2020. Let's mark this PR blocked until then.

aduh95 · 2022-03-23T14:17:07Z

@jeromecovington With Node.js v18.0.0 around the corner and Node.js v12.x fading to EOL next month, I think we should consider landing this soon. Would you be available to rebase your branch on top of master and fix all the conflicts?

jeromecovington · 2022-03-23T14:38:52Z

@aduh95 from what I can tell the conflicts are mostly around the addition of cjs/mjs dependency-style code blocks. I should have some time to take care of that later this week or this weekend. Thanks for the heads up.

jeromecovington · 2022-03-28T00:58:15Z

The conflicts are now so significant that I think we'll have better luck just cutting a fresh branch off of master and redoing this work, rather than working toward resolving. I can take a look at that later or if someone else has time, I'm happy to shut down this PR in favor of a more up to date approach.

aduh95 · 2022-03-28T07:26:07Z

The conflicts are now so significant that I think we'll have better luck just cutting a fresh branch off of master and redoing this work, rather than working toward resolving. I can take a look at that later or if someone else has time, I'm happy to shut down this PR in favor of a more up to date approach.

FWIW you don't have to create a new PR to start fresh. You can run the following command when on this PR branch:

git fetch https://github.com/nodejs/node.git master
git reset FETCH_HEAD --hard
# now your local git repo is in a fresh state

# and when you're done:
git commit
git push --force-with-lease

That being said, if creating a new PR is more convenient for you, you can certainly do that – whatever you do, I'm grateful for the work you're putting to improve Node.js docs :)

GeoffreyBooth · 2022-03-28T16:14:54Z

You can run the following command

We should add this to the contributors’ guide.

github-actions bot added the doc Issues and PRs related to the documentations. label May 10, 2021

jeromecovington mentioned this pull request May 10, 2021

Use the node: protocol in docs examples #38343

Closed

Trott approved these changes May 12, 2021

View reviewed changes

Trott added dont-land-on-v12.x labels May 16, 2021

DerekNonGeneric approved these changes May 16, 2021

View reviewed changes

DerekNonGeneric previously requested changes May 17, 2021

View reviewed changes

DerekNonGeneric added the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label May 17, 2021

aymen94 approved these changes May 17, 2021

View reviewed changes

jeromecovington added 3 commits May 18, 2021 19:39

doc: use node protocol in ESM examples

a4fec89

doc: use node protocol in CommonJS examples

ebb4ff0

doc: ensure line <= 80 for markdown linter

8e6349f

jeromecovington force-pushed the node-protocol-api-docs-2 branch from b2b6fa5 to 8e6349f Compare May 18, 2021 23:46

doc: remove errant usage of node:cjs

7737f62

DerekNonGeneric removed the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label May 19, 2021

aduh95 added backport-blocked-v14.x and removed dont-land-on-v14.x labels May 19, 2021

targos removed the dont-land-on-v15.x label Jun 5, 2021

aduh95 removed the backport-blocked-v14.x label Dec 17, 2021

jasnell reviewed Dec 19, 2021

View reviewed changes

jasnell added the blocked PRs that are blocked by other issues or PRs. label Dec 19, 2021

aduh95 mentioned this pull request Mar 23, 2022

module: ensure 'node:'-only modules can access node_modules #42430

Merged

aduh95 mentioned this pull request Apr 15, 2022

doc: add node: prefix for all core modules #42752

Merged

jeromecovington closed this Apr 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add node protocol to ESM and CommonJS api examples #38620

doc: add node protocol to ESM and CommonJS api examples #38620

jeromecovington commented May 10, 2021

jeromecovington commented May 10, 2021

Trott left a comment

Trott commented May 16, 2021

DerekNonGeneric left a comment •

edited

Loading

DerekNonGeneric commented May 17, 2021

Trott commented May 17, 2021

DerekNonGeneric commented May 17, 2021

DerekNonGeneric left a comment

DerekNonGeneric May 17, 2021

jeromecovington May 17, 2021

jeromecovington May 18, 2021

DerekNonGeneric May 19, 2021

targos commented May 17, 2021 •

edited

Loading

Trott commented May 19, 2021

Trott commented May 23, 2021

Trott commented May 23, 2021

jasnell commented May 23, 2021

Trott commented Dec 17, 2021

guybedford commented Dec 17, 2021

ljharb commented Dec 17, 2021

aduh95 commented Dec 17, 2021

ljharb commented Dec 17, 2021

jasnell left a comment

aduh95 commented Mar 23, 2022

jeromecovington commented Mar 23, 2022

jeromecovington commented Mar 28, 2022

aduh95 commented Mar 28, 2022

GeoffreyBooth commented Mar 28, 2022

doc: add node protocol to ESM and CommonJS api examples #38620

doc: add node protocol to ESM and CommonJS api examples #38620

Conversation

jeromecovington commented May 10, 2021

jeromecovington commented May 10, 2021

Trott left a comment

Choose a reason for hiding this comment

Trott commented May 16, 2021

DerekNonGeneric left a comment • edited Loading

Choose a reason for hiding this comment

DerekNonGeneric commented May 17, 2021

Trott commented May 17, 2021

DerekNonGeneric commented May 17, 2021

DerekNonGeneric left a comment

Choose a reason for hiding this comment

DerekNonGeneric May 17, 2021

Choose a reason for hiding this comment

jeromecovington May 17, 2021

Choose a reason for hiding this comment

jeromecovington May 18, 2021

Choose a reason for hiding this comment

DerekNonGeneric May 19, 2021

Choose a reason for hiding this comment

targos commented May 17, 2021 • edited Loading

Trott commented May 19, 2021

Trott commented May 23, 2021

Trott commented May 23, 2021

jasnell commented May 23, 2021

Trott commented Dec 17, 2021

guybedford commented Dec 17, 2021

ljharb commented Dec 17, 2021

aduh95 commented Dec 17, 2021

ljharb commented Dec 17, 2021

jasnell left a comment

Choose a reason for hiding this comment

aduh95 commented Mar 23, 2022

jeromecovington commented Mar 23, 2022

jeromecovington commented Mar 28, 2022

aduh95 commented Mar 28, 2022

GeoffreyBooth commented Mar 28, 2022

DerekNonGeneric left a comment •

edited

Loading

targos commented May 17, 2021 •

edited

Loading