Skip to content

Conversation

@aduh95
Copy link
Contributor

Using a "Modules:" prefix groups all the related pages together when
using alphabetical order.

This is a first PR in attempt to improve package documentation. More changes are coming as discussed in nodejs/modules#539, I've decided to split into several PRs to ease the code review.

Refs: nodejs/modules#539

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the tools Issues and PRs related to the tools directory. label Aug 7, 2020
Copy link
Member

@jasnelljasnell left a comment

Choose a reason for hiding this comment

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

I'm generally -1 on this given that it will break existing links to docs in http://nodejs.org. If we can find a way of aliasing renamed pages first, so that existing links can be forwarded, that would be ideal.

@guybedford
Copy link
Contributor

Agreed it should be possible to do this without changing the existing URLs. Most importantly for the CommonJS modules.md section, but also for the esm.md section. Could we just change the title texts?

@aduh95aduh95force-pushed the module-pages-renaming branch from 9898afc to 41b2a11CompareAugust 7, 2020 21:23
@aduh95aduh95 requested a review from jasnellAugust 7, 2020 21:24
@aduh95
Copy link
ContributorAuthor

That's right, I haven't thought of that! I have implemented @guybedford's suggestion, PTAL.

Copy link
Contributor

@guybedfordguybedford left a comment

Choose a reason for hiding this comment

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

This looks like a good start to me, in particular renaming the existing Modules section to Modules: CommonJS Modules seems like an important change to get through at this point.

@GeoffreyBooth
Copy link
Member

@jasnell Do you know who the best person to talk to would be about the mechanics of the docs generation? In particular I'm wondering if anyone can advise us on how to create redirect links so that we can feel free to move or rename sections without breaking links.

@jasnell
Copy link
Member

Unfortunately no. I've never done any digging into that part of the code

@richardlau
Copy link
Member

Docs are built statically as part of the release process and then copied across to the staging server (see

node/Makefile

Lines 1092 to 1097 in bcfb176

# Note: this is strictly for release builds on release machines only.
doc-upload: doc
ssh $(STAGINGSERVER)"mkdir -p nodejs/$(DISTTYPEDIR)/$(FULLVERSION)/docs/"
chmod -R ug=rw-x+X,o=r+X out/doc/
scp -pr out/doc/*$(STAGINGSERVER):nodejs/$(DISTTYPEDIR)/$(FULLVERSION)/docs/
ssh $(STAGINGSERVER)"touch nodejs/$(DISTTYPEDIR)/$(FULLVERSION)/docs.done"
) (they go live when the release is promoted). When the promotion happens the appropriate latest- link (see https://nodejs.org/docs/) is updated to point to the version uploaded.

I think redirects for the website are handled via https://github.com/nodejs/build/blob/master/ansible/www-standalone/resources/config/nodejs.org. That's completely outside of the docs generation process and I'm not sure there's that many people who understand how that's deployed.

@aduh95
Copy link
ContributorAuthor

@jasnell I've updated the PR to address your comment, would be willing to dismiss your objection or to rephrase it if this PR still needs work from me?

@aduh95
Copy link
ContributorAuthor

Could this be labeled Author ready?

@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2020
@GeoffreyBooth
Copy link
Member

@aduh95 This needs a rebase, probably because of #34747.

Using a "Modules:" prefix groups all the related pages together when using alphabetical order. Refs: nodejs/modules#539
@aduh95aduh95force-pushed the module-pages-renaming branch from 41b2a11 to 80bea04CompareAugust 19, 2020 18:07
@aduh95
Copy link
ContributorAuthor

Rebased on master, should be good now!

@TrottTrott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 20, 2020
Copy link
Member

@GeoffreyBoothGeoffreyBooth left a comment

Choose a reason for hiding this comment

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

This looks good to me. @Trott are you okay with the current language?

@Trott
Copy link
Member

This looks good to me. @Trott are you okay with the current language?

Yes.

@Trott
Copy link
Member

Landed in 22e3ada

Trott pushed a commit that referenced this pull request Aug 23, 2020
Using a "Modules:" prefix groups all the related pages together when using alphabetical order. Refs: nodejs/modules#539 PR-URL: #34663 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@TrottTrott closed this Aug 23, 2020
@aduh95aduh95 mentioned this pull request Aug 24, 2020
BethGriggs pushed a commit that referenced this pull request Aug 24, 2020
Using a "Modules:" prefix groups all the related pages together when using alphabetical order. Refs: nodejs/modules#539 PR-URL: #34663 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>
aduh95 added a commit to aduh95/node that referenced this pull request Oct 23, 2020
Using a "Modules:" prefix groups all the related pages together when using alphabetical order. Refs: nodejs/modules#539 PR-URL: nodejs#34663 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@aduh95aduh95 deleted the module-pages-renaming branch October 28, 2020 14:53
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
Using a "Modules:" prefix groups all the related pages together when using alphabetical order. Refs: nodejs/modules#539 Backport-PR-URL: #35757 PR-URL: #34663 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
Using a "Modules:" prefix groups all the related pages together when using alphabetical order. Refs: nodejs/modules#539 Backport-PR-URL: #35757 PR-URL: #34663 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.esmIssues and PRs related to the ECMAScript Modules implementation.moduleIssues and PRs related to the module subsystem.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@aduh95@guybedford@GeoffreyBooth@jasnell@richardlau@Trott@trivikr@DerekNonGeneric@nodejs-github-bot