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

tools,doc: avoid generating duplicate id attributes #41291

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 23, 2021

In all.html, we currently generate hundreds of duplicate id attributes
because of conflicts between the way allhtml.mjs prefixes in-page links
with the module name on the one hand, and the existence of legacy id
attributes hardcoded into the page on the other hand.

This prefaces the module name with all_ to avoid the conflicts.

Use autogenerated id attributes.
In all.html, we currently generate hundreds of duplicate id attributes
because of conflicts between the way allhtml.mjs prefixes in-page links
with the module name on the one hand, and the existence of legacy id
attributes hardcoded into the page on the other hand.

This prefaces the module name with `all_` to avoid the conflicts.
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2021
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Dec 23, 2021
@Trott Trott added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Dec 23, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Dec 23, 2021

This prefaces the module name with all_ to avoid the conflicts.

Wouldn't that break existing links? (like a link from a StackOverflow post that lead to Node.js docs)

@Trott
Copy link
Member Author

Trott commented Dec 23, 2021

This prefaces the module name with all_ to avoid the conflicts.

Wouldn't that break existing links? (like a link from a StackOverflow post that lead to Node.js docs)

Two responses:

  1. Yes, sometimes, but often not. There are lots of duplicate IDs being generated so the duplicates are still there and working.
  2. And for the ones that do break: I'm not concerned, honestly. This is in all.html, a 6.6 Mb file that is mostly there for people that need to do "command F" searching the docs and for people who like to easily print their documentation on 100s of pages of paper. Most links from StackOverflow and elsewhere go do the individual module pages instead. Those links are unaffected.

@Trott
Copy link
Member Author

Trott commented Dec 23, 2021

  • Yes, sometimes, but often not. There are lots of duplicate IDs being generated so the duplicates are still there and working.
  • And for the ones that do break: I'm not concerned, honestly. This is in all.html, a 6.6 Mb file that is mostly there for people that need to do "command F" searching the docs and for people who like to easily print their documentation on 100s of pages of paper. Most links from StackOverflow and elsewhere go do the individual module pages instead. Those links are unaffected.

Oh, and 3.: This won't affect links in previous versions. So only relatively recent links to https://nodejs.org/dist/latest-v17.x/docs/api/all.html. If we want, we can decide not to land this on LTS to preserve the links in https://nodejs.org/dist/latest-v16.x/docs/api/all.html and earlier. And for v18.x and later, the non-all-prefixed links will never have existed in the first place.

So on balance, I consider the breaking possibilities fairly minimal. (Plus the link still "works", it just won't scroll you to the right place in the doc. So you'll have to command-f search or whatever.)

@aduh95
Copy link
Contributor

aduh95 commented Dec 23, 2021

we can decide not to land this on LTS to preserve the links in https://nodejs.org/dist/latest-v16.x/docs/api/all.html and earlier

#39304 only landed on v16.12.0 which is two months old, and wasn't backported to earlier release lines. Yeah you're right, it's probably fine to land this and even have it landed on v16.x sooner rather than later.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v12.x labels Dec 23, 2021
@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 5cc4b69...83f442a

nodejs-github-bot pushed a commit that referenced this pull request Dec 25, 2021
Use autogenerated id attributes.

PR-URL: #41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Dec 25, 2021
In all.html, we currently generate hundreds of duplicate id attributes
because of conflicts between the way allhtml.mjs prefixes in-page links
with the module name on the one hand, and the existence of legacy id
attributes hardcoded into the page on the other hand.

This prefaces the module name with `all_` to avoid the conflicts.

PR-URL: #41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
Use autogenerated id attributes.

PR-URL: #41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
In all.html, we currently generate hundreds of duplicate id attributes
because of conflicts between the way allhtml.mjs prefixes in-page links
with the module name on the one hand, and the existence of legacy id
attributes hardcoded into the page on the other hand.

This prefaces the module name with `all_` to avoid the conflicts.

PR-URL: #41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Use autogenerated id attributes.

PR-URL: #41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
In all.html, we currently generate hundreds of duplicate id attributes
because of conflicts between the way allhtml.mjs prefixes in-page links
with the module name on the one hand, and the existence of legacy id
attributes hardcoded into the page on the other hand.

This prefaces the module name with `all_` to avoid the conflicts.

PR-URL: #41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Use autogenerated id attributes.

PR-URL: nodejs#41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
In all.html, we currently generate hundreds of duplicate id attributes
because of conflicts between the way allhtml.mjs prefixes in-page links
with the module name on the one hand, and the existence of legacy id
attributes hardcoded into the page on the other hand.

This prefaces the module name with `all_` to avoid the conflicts.

PR-URL: nodejs#41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Use autogenerated id attributes.

PR-URL: #41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
In all.html, we currently generate hundreds of duplicate id attributes
because of conflicts between the way allhtml.mjs prefixes in-page links
with the module name on the one hand, and the existence of legacy id
attributes hardcoded into the page on the other hand.

This prefaces the module name with `all_` to avoid the conflicts.

PR-URL: #41291
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
@Trott Trott deleted the duplicate-id-attrs branch September 25, 2022 17:12
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. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants