fix(cli): write-heading-id should not generate colliding slugs when not overwriting #6849
Conversation
| @@ -38,77 +38,60 @@ function addHeadingId( | |||
|
|
|||
| const headingText = line.slice(headingLevel).trimEnd(); | |||
| const headingHashes = line.slice(0, headingLevel); | |||
| const slug = slugger | |||
| .slug(unwrapMarkdownLinks(headingText).trim(), {maintainCase}) | |||
| .replace(/^-+/, '') | |||
There was a problem hiding this comment.
This also has the possibility of generating colliding slugs, so it's removed. We would probably fix it inside the slugger.slug function by first filtering out the illegal characters
| // If we can't overwrite existing slugs, make sure other headings don't | ||
| // generate colliding slugs by first marking these slugs as occupied | ||
| if (!overwrite) { | ||
| lines.forEach((line) => { | ||
| const parsedHeading = parseMarkdownHeadingId(line); | ||
| if (parsedHeading.id) { | ||
| slugger.slug(parsedHeading.id); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Most of this is refactor and removing excess layers of abstraction, and this double-pass slugification is the only fix I made
There was a problem hiding this comment.
👍 nice trick
also wanted to use something similar unfortunately afaik the slugger does not offer an API to register forbidden slugs or something 😅
|
✔️ [V2] 🔨 Explore the source changes: a69ce38 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6223295b1a436f0007084b41 😎 Browse the preview: https://deploy-preview-6849--docusaurus-2.netlify.app |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6849--docusaurus-2.netlify.app/ |
|
Size Change: 0 B Total Size: 791 kB ℹ️ View Unchanged
|
| const expected = ` | ||
|
|
||
| # Ignored title | ||
|
|
||
| ## abc {#abc} | ||
| ## abc {#abc-1} |
| // If we can't overwrite existing slugs, make sure other headings don't | ||
| // generate colliding slugs by first marking these slugs as occupied | ||
| if (!overwrite) { | ||
| lines.forEach((line) => { | ||
| const parsedHeading = parseMarkdownHeadingId(line); | ||
| if (parsedHeading.id) { | ||
| slugger.slug(parsedHeading.id); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
👍 nice trick
also wanted to use something similar unfortunately afaik the slugger does not offer an API to register forbidden slugs or something 😅
Motivation
Complete a TODO. This probably doesn't happen a lot, but better fix the edge cases
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
The test case is updated