Skip to content

tools,doc: avoid generating duplicate id attributes - #41291

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:duplicate-id-attrs
Closed

tools,doc: avoid generating duplicate id attributes#41291
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:duplicate-id-attrs

Conversation

@Trott

Copy link
Copy Markdown
Member

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.
@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2021
@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Dec 23, 2021
@TrottTrott 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-actionsgithub-actionsBot 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
Copy Markdown
Collaborator

@aduh95

Copy link
Copy Markdown
Contributor

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
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor
  • 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
Copy Markdown
Contributor

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.

@aduh95aduh95 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
@TrottTrott added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2021
@nodejs-github-botnodejs-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
Copy Markdown
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>
@targostargos mentioned this pull request Jan 16, 2022
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>
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>
@danielleadamsdanielleadams mentioned this pull request Feb 1, 2022
@Trott
Trott deleted the duplicate-id-attrs branch September 25, 2022 17:12
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.docIssues and PRs related to the documentations.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Trott@nodejs-github-bot@aduh95@jasnell@tniessen