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

 <!-- eslint-disable no-duplicate-imports -->
 ```js
-import{defaultascjs } from'cjs';
+import{defaultascjs } 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!

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?

@aduh95node: 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-actionsbot 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

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 8e6349fCompare May 18, 2021 23:46

doc: remove errant usage of node:cjs
7737f62

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

Uh oh!

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

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

Uh oh!

Conversation

jeromecovington commented May 10, 2021

Uh oh!

jeromecovington commented May 10, 2021

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented May 16, 2021

Uh oh!

DerekNonGeneric left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DerekNonGeneric commented May 17, 2021

Uh oh!

Trott commented May 17, 2021

Uh oh!

DerekNonGeneric commented May 17, 2021

Uh oh!

DerekNonGeneric left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DerekNonGenericMay 17, 2021

Choose a reason for hiding this comment

Uh oh!

jeromecovingtonMay 17, 2021

Choose a reason for hiding this comment

Uh oh!

jeromecovingtonMay 18, 2021

Choose a reason for hiding this comment

Uh oh!

DerekNonGenericMay 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

targos commented May 17, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented May 19, 2021

Uh oh!

Trott commented May 23, 2021

Uh oh!

Trott commented May 23, 2021

Uh oh!

jasnell commented May 23, 2021

Uh oh!

Trott commented Dec 17, 2021

Uh oh!

guybedford commented Dec 17, 2021

Uh oh!

ljharb commented Dec 17, 2021

Uh oh!

aduh95 commented Dec 17, 2021

Uh oh!

ljharb commented Dec 17, 2021

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Mar 23, 2022

Uh oh!

jeromecovington commented Mar 23, 2022

Uh oh!

jeromecovington commented Mar 28, 2022

Uh oh!

aduh95 commented Mar 28, 2022

Uh oh!

GeoffreyBooth commented Mar 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

DerekNonGeneric left a comment •
edited
Loading

targos commented May 17, 2021•
edited
Loading