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

doc: anchor link parity between markdown and html-generated docs #39304

Closed
wants to merge 17 commits into from

Conversation

foxxyz
Copy link
Contributor

@foxxyz foxxyz commented Jul 8, 2021

Inspired by @aduh95's comments on #39164.

Honestly, this is probably a long shot due to the amount of changes, but I think the benefits are substantial - so here goes.

Main Changes

  • Replace current HTML anchor generation to match header anchor generation in Github markdown
  • Remove unnecessary double namespacing on generated anchors/links (E.G. esm.md#loaders instead of esm.md#esm_loaders)
  • Anchors/links are automatically prefixed with their respective modules when concatenated for usage in all.html

Benefits

  • All anchor links within and between markdown API docs actually work (try it out here)
  • Adding new anchor links no longer requires contributors to generate the HTML docs first to look up the correct anchors
  • Anchors are much shorter
  • Simpler anchor generation
  • All previous anchor links are preserved by generating hidden legacy anchors.

Linting (make lint-md) and doc generation (make doc-only) should have verified all new links are correct, so hopefully that saves you from needing to double-check 95% of changes in this PR. The most important changes are to files in tools/doc.

I realize this will mean tons of rebasing if this PR stays open, but I don't mind doing it if there's a chance links in the markdown docs might actually work at some point.

Other issues that discuss this: #35189 (specific comment)

@github-actions github-actions bot added the doc Issues and PRs related to the documentations. label Jul 8, 2021
@aduh95

This comment has been minimized.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for putting the effort into this, that's good work! I am a bit concerned regarding breaking existing links, but maybe the upside is worth the potential breaking. Maybe we could add something similar to what we did at https://nodejs.org/docs/latest/api/modules.html#modules_module_builtinmodules?

Alternatively, have you looked into https://www.markdownguide.org/extended-syntax/#heading-ids as an alternative? If GitHub web UI supports those, that would let us keep our existing links for the same benefit – the downside would be to have to add those heading-ids in our .md files.

tools/doc/allhtml.mjs Outdated Show resolved Hide resolved
tools/doc/allhtml.mjs Outdated Show resolved Hide resolved
@foxxyz
Copy link
Contributor Author

foxxyz commented Jul 9, 2021

Alternatively, have you looked into https://www.markdownguide.org/extended-syntax/#heading-ids as an alternative? If GitHub web UI supports those, that would let us keep our existing links for the same benefit – the downside would be to have to add those heading-ids in our .md files.

That was the first thing I tried :) Unfortunately it's not supported in the Github markdown renderer, it just renders it next to the heading.

I am a bit concerned regarding breaking existing links, but maybe the upside is worth the potential breaking. Maybe we could add something similar to what we did at https://nodejs.org/docs/latest/api/modules.html#modules_module_builtinmodules?

I think I have a solution: 24df945 adds alias anchors to each heading that reflect the old ID scheme, while keeping the markdown uncluttered. I think that should preserve all incoming links! Let me know if you think the HTML should be different.

tools/doc/html.mjs Outdated Show resolved Hide resolved
@foxxyz foxxyz force-pushed the consistent-doc-anchors branch 2 times, most recently from f58ccf2 to 50b726f Compare July 13, 2021 06:05
tools/doc/allhtml.mjs Outdated Show resolved Hide resolved
tools/doc/allhtml.mjs Outdated Show resolved Hide resolved
@foxxyz foxxyz force-pushed the consistent-doc-anchors branch 5 times, most recently from af39af5 to f39d7b7 Compare July 20, 2021 06:32
@foxxyz foxxyz requested review from jasnell and aduh95 July 20, 2021 06:32
tools/doc/allhtml.mjs Outdated Show resolved Hide resolved
tools/doc/allhtml.mjs Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Jul 21, 2021

@nodejs/documentation wdyt?
Also, @nodejs/lts this could impact backporting doc changes.

@foxxyz foxxyz force-pushed the consistent-doc-anchors branch 3 times, most recently from 848b7e5 to b300764 Compare July 27, 2021 05:21
@foxxyz
Copy link
Contributor Author

foxxyz commented Aug 4, 2021

@aduh95 @jasnell anything I can do to increase chances of feedback and/or adoption?

@aduh95 aduh95 added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 4, 2021
@aduh95
Copy link
Contributor

aduh95 commented Aug 4, 2021

I've added the tsc-agenda label to get more feedback and to discuss when would be the best time to land this.

@aduh95 aduh95 added backport-requested-v14.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed backport-blocked-v14.x labels Aug 29, 2021
aduh95 pushed a commit that referenced this pull request Aug 29, 2021
Main changes:

- Replace current HTML anchor generation to match
  header anchor generation in Github markdown.
- Remove unnecessary double namespacing on generated anchors/links (E.G.
  `esm.md#loaders` instead of `esm.md#esm_loaders`).
- Anchors/links are automatically prefixed with their respective modules
  when concatenated for usage in `all.html`.

Benefits:

- All anchor links within and between markdown API docs actually work.
- Adding new anchor links no longer requires contributors to generate
  the HTML docs first to look up the correct anchors.
- Anchors are much shorter.
- All previous anchor links are preserved by generating hidden legacy
  anchors.

PR-URL: #39304
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Aug 29, 2021

Landed in 6fdd582.

@aduh95 aduh95 closed this Aug 29, 2021
@aduh95
Copy link
Contributor

aduh95 commented Aug 29, 2021

Thanks again for your hard work on this. Would you like to open a backport PR for v14.x? The commands to achieve that would be:

git fetch upstream master
git fetch upstream v14.x-staging
git reset upstream/v14.x-staging --hard
git cherry-pick 6fdd5827f0956ffc4e7ffe31babaf530e42f75b9

@foxxyz
Copy link
Contributor Author

foxxyz commented Aug 30, 2021

Awesome!

Thanks again for your hard work on this. Would you like to open a backport PR for v14.x?

I'd love to.

@foxxyz foxxyz deleted the consistent-doc-anchors branch August 30, 2021 01:16
@targos
Copy link
Member

targos commented Aug 30, 2021

Please don't backport yet. It would introduce massive conflicts with commits that haven't landed yet on v14.x

@foxxyz
Copy link
Contributor Author

foxxyz commented Aug 30, 2021

I expect there to be some big conflicts - I don't mind working through them - but happy to wait for your go-ahead before submitting the backport PR, @targos. Let me know.

@foxxyz
Copy link
Contributor Author

foxxyz commented Sep 26, 2021

@targos any change? is there still a desire to have this backported?

targos pushed a commit that referenced this pull request Oct 9, 2021
Main changes:

- Replace current HTML anchor generation to match
  header anchor generation in Github markdown.
- Remove unnecessary double namespacing on generated anchors/links (E.G.
  `esm.md#loaders` instead of `esm.md#esm_loaders`).
- Anchors/links are automatically prefixed with their respective modules
  when concatenated for usage in `all.html`.

Benefits:

- All anchor links within and between markdown API docs actually work.
- Adding new anchor links no longer requires contributors to generate
  the HTML docs first to look up the correct anchors.
- Anchors are much shorter.
- All previous anchor links are preserved by generating hidden legacy
  anchors.

PR-URL: #39304
Reviewed-By: Antoine du Hamel <[email protected]>
@targos
Copy link
Member

targos commented Oct 9, 2021

There were little conflicts on v16.x-staging and I fixed them. Pushed at d3f3111

@targos
Copy link
Member

targos commented Oct 9, 2021

About v14.x, I would be happy to review a backport pull request, but the branch is going into maintenance in less than two weeks, so maybe it's not worth the work.

danielleadams pushed a commit that referenced this pull request Oct 12, 2021
Main changes:

- Replace current HTML anchor generation to match
  header anchor generation in Github markdown.
- Remove unnecessary double namespacing on generated anchors/links (E.G.
  `esm.md#loaders` instead of `esm.md#esm_loaders`).
- Anchors/links are automatically prefixed with their respective modules
  when concatenated for usage in `all.html`.

Benefits:

- All anchor links within and between markdown API docs actually work.
- Adding new anchor links no longer requires contributors to generate
  the HTML docs first to look up the correct anchors.
- Anchors are much shorter.
- All previous anchor links are preserved by generating hidden legacy
  anchors.

PR-URL: #39304
Reviewed-By: Antoine du Hamel <[email protected]>
foxxyz added a commit to foxxyz/node that referenced this pull request Oct 18, 2021
Main changes:

- Replace current HTML anchor generation to match
  header anchor generation in Github markdown.
- Remove unnecessary double namespacing on generated anchors/links (E.G.
  `esm.md#loaders` instead of `esm.md#esm_loaders`).
- Anchors/links are automatically prefixed with their respective modules
  when concatenated for usage in `all.html`.

Benefits:

- All anchor links within and between markdown API docs actually work.
- Adding new anchor links no longer requires contributors to generate
  the HTML docs first to look up the correct anchors.
- Anchors are much shorter.
- All previous anchor links are preserved by generating hidden legacy
  anchors.

PR-URL: nodejs#39304
Reviewed-By: Antoine du Hamel <[email protected]>
foxxyz added a commit to foxxyz/node that referenced this pull request Oct 18, 2021
Main changes:

- Replace current HTML anchor generation to match
  header anchor generation in Github markdown.
- Remove unnecessary double namespacing on generated anchors/links (E.G.
  `esm.md#loaders` instead of `esm.md#esm_loaders`).
- Anchors/links are automatically prefixed with their respective modules
  when concatenated for usage in `all.html`.

Benefits:

- All anchor links within and between markdown API docs actually work.
- Adding new anchor links no longer requires contributors to generate
  the HTML docs first to look up the correct anchors.
- Anchors are much shorter.
- All previous anchor links are preserved by generating hidden legacy
  anchors.

PR-URL: nodejs#39304
Reviewed-By: Antoine du Hamel <[email protected]>
foxxyz added a commit to foxxyz/node that referenced this pull request Oct 18, 2021
Main changes:

- Replace current HTML anchor generation to match
  header anchor generation in Github markdown.
- Remove unnecessary double namespacing on generated anchors/links (E.G.
  `esm.md#loaders` instead of `esm.md#esm_loaders`).
- Anchors/links are automatically prefixed with their respective modules
  when concatenated for usage in `all.html`.

Benefits:

- All anchor links within and between markdown API docs actually work.
- Adding new anchor links no longer requires contributors to generate
  the HTML docs first to look up the correct anchors.
- Anchors are much shorter.
- All previous anchor links are preserved by generating hidden legacy
  anchors.

PR-URL: nodejs#39304
Reviewed-By: Antoine du Hamel <[email protected]>
foxxyz added a commit to foxxyz/node that referenced this pull request Oct 18, 2021
Main changes:

- Replace current HTML anchor generation to match
  header anchor generation in Github markdown.
- Remove unnecessary double namespacing on generated anchors/links (E.G.
  `esm.md#loaders` instead of `esm.md#esm_loaders`).
- Anchors/links are automatically prefixed with their respective modules
  when concatenated for usage in `all.html`.

Benefits:

- All anchor links within and between markdown API docs actually work.
- Adding new anchor links no longer requires contributors to generate
  the HTML docs first to look up the correct anchors.
- Anchors are much shorter.
- All previous anchor links are preserved by generating hidden legacy
  anchors.

PR-URL: nodejs#39304
Reviewed-By: Antoine du Hamel <[email protected]>
@foxxyz
Copy link
Contributor Author

foxxyz commented Oct 18, 2021

About v14.x, I would be happy to review a backport pull request, but the branch is going into maintenance in less than two weeks, so maybe it's not worth the work.

Thank you for merging into 16.x, hope it wasn't too cumbersome! 🙌🏽

I've submitted a PR for the backport at #40495 , hopefully I'm not too late.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants