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

doc: rename module pages #34663

Closed
wants to merge 2 commits into from
Closed

doc: rename module pages #34663

wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 7, 2020

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-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Aug 7, 2020
jasnell
jasnell requested changes Aug 7, 2020
View reviewed changes
Copy link
Member

@jasnell jasnell 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?

@aduh95 aduh95 force-pushed the module-pages-renaming branch from 9898afc to 41b2a11 Compare August 7, 2020 21:23
@aduh95 aduh95 requested a review from jasnell August 7, 2020 21:24
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 7, 2020

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

guybedford
guybedford approved these changes Aug 8, 2020
View reviewed changes
Copy link
Contributor

@guybedford guybedford 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

jasnell commented Aug 10, 2020

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.

@DerekNonGeneric DerekNonGeneric added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. labels Aug 11, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 12, 2020

@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 aduh95 mentioned this pull request Aug 18, 2020
Closed
3 tasks
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 19, 2020

Could this be labeled Author ready?

@jasnell jasnell 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.

@aduh95
doc: rename module pages
80bea04 
Using a "Modules:" prefix groups all the related pages together when
using alphabetical order.

Refs: nodejs/modules#539
@aduh95 aduh95 force-pushed the module-pages-renaming branch from 41b2a11 to 80bea04 Compare August 19, 2020 18:07
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 19, 2020

Rebased on master, should be good now!

Trott
Trott reviewed Aug 20, 2020
View reviewed changes
doc/api/index.md Outdated Show resolved Hide resolved
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 20, 2020
GeoffreyBooth
GeoffreyBooth approved these changes Aug 22, 2020
View reviewed changes
Copy link
Member

@GeoffreyBooth GeoffreyBooth 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

Trott commented Aug 23, 2020

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

Yes.

@Trott
Copy link
Member

Trott commented Aug 23, 2020

Landed in 22e3ada

Trott pushed a commit that referenced this pull request Aug 23, 2020
@aduh95 @Trott
doc: rename module pages
22e3ada 
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 <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott closed this Aug 23, 2020
@aduh95 aduh95 mentioned this pull request Aug 24, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Aug 24, 2020
@aduh95 @BethGriggs
doc: rename module pages
bf7f492 
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 <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 23, 2020
Closed
3 tasks
aduh95 added a commit to aduh95/node that referenced this pull request Oct 23, 2020
@aduh95
doc: rename module pages
4c714c7 
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 <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 aduh95 deleted the module-pages-renaming branch October 28, 2020 14:53
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
@aduh95 @MylesBorins
doc: rename module pages
8b55581 
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 <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@aduh95 @MylesBorins
doc: rename module pages
7510667 
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 <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@Trott Trott Trott approved these changes

@guybedford guybedford guybedford approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@aduh95 @guybedford @GeoffreyBooth @jasnell @richardlau @Trott @trivikr @DerekNonGeneric @nodejs-github-bot