fix(cli): write-heading-id should not generate colliding slugs when not overwriting

packages/docusaurus/src/commands/__tests__/writeHeadingIds.test.ts

@@ -5,91 +5,63 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {
-  transformMarkdownHeadingLine,
-  transformMarkdownContent,
-} from '../writeHeadingIds';
-import {createSlugger} from '@docusaurus/utils';
-
-describe('transformMarkdownHeadingLine', () => {
-  test('throws when not a heading', () => {
-    expect(() =>
-      transformMarkdownHeadingLine('ABC', createSlugger()),
-    ).toThrowErrorMatchingInlineSnapshot(
-      `"Line is not a Markdown heading: ABC."`,
-    );
-  });
+import {transformMarkdownContent} from '../writeHeadingIds';
 
+describe('transformMarkdownContent', () => {
   test('works for simple level-2 heading', () => {
-    expect(transformMarkdownHeadingLine('## ABC', createSlugger())).toEqual(
-      '## ABC {#abc}',
-    );
+    expect(transformMarkdownContent('## ABC')).toEqual('## ABC {#abc}');
   });
 
   test('works for simple level-3 heading', () => {
-    expect(transformMarkdownHeadingLine('### ABC', createSlugger())).toEqual(
-      '### ABC {#abc}',
-    );
+    expect(transformMarkdownContent('### ABC')).toEqual('### ABC {#abc}');
   });
 
   test('works for simple level-4 heading', () => {
-    expect(transformMarkdownHeadingLine('#### ABC', createSlugger())).toEqual(
-      '#### ABC {#abc}',
-    );
+    expect(transformMarkdownContent('#### ABC')).toEqual('#### ABC {#abc}');
   });
 
   test('unwraps markdown links', () => {
     const input = `## hello [facebook](https://facebook.com) [crowdin](https://crowdin.com/translate/docusaurus-v2/126/en-fr?filter=basic&value=0)`;
-    expect(transformMarkdownHeadingLine(input, createSlugger())).toEqual(
+    expect(transformMarkdownContent(input)).toEqual(
       `${input} {#hello-facebook-crowdin}`,
     );
   });
 
   test('can slugify complex headings', () => {
     const input = '## abc [Hello] How are you %Sébastien_-_$)( ## -56756';
-    expect(transformMarkdownHeadingLine(input, createSlugger())).toEqual(
+    expect(transformMarkdownContent(input)).toEqual(
       `${input} {#abc-hello-how-are-you-sébastien_-_---56756}`,
     );
   });
 
   test('does not duplicate duplicate id', () => {
-    expect(
-      transformMarkdownHeadingLine(
-        '## hello world {#hello-world}',
-        createSlugger(),
-      ),
-    ).toEqual('## hello world {#hello-world}');
+    expect(transformMarkdownContent('## hello world {#hello-world}')).toEqual(
+      '## hello world {#hello-world}',
+    );
   });
 
   test('respects existing heading', () => {
-    expect(
-      transformMarkdownHeadingLine(
-        '## New heading {#old-heading}',
-        createSlugger(),
-      ),
-    ).toEqual('## New heading {#old-heading}');
+    expect(transformMarkdownContent('## New heading {#old-heading}')).toEqual(
+      '## New heading {#old-heading}',
+    );
   });
 
   test('overwrites heading ID when asked to', () => {
     expect(
-      transformMarkdownHeadingLine(
-        '## New heading {#old-heading}',
-        createSlugger(),
-        {overwrite: true},
-      ),
+      transformMarkdownContent('## New heading {#old-heading}', {
+        overwrite: true,
+      }),
     ).toEqual('## New heading {#new-heading}');
   });
 
   test('maintains casing when asked to', () => {
     expect(
-      transformMarkdownHeadingLine('## getDataFromAPI()', createSlugger(), {
+      transformMarkdownContent('## getDataFromAPI()', {
         maintainCase: true,
       }),
     ).toEqual('## getDataFromAPI() {#getDataFromAPI}');
   });
-});
 
-describe('transformMarkdownContent', () => {
   test('transform the headings', () => {
     const input = `
 
@@ -113,14 +85,11 @@ describe('transformMarkdownContent', () => {
 
     `;
 
-    // TODO the first heading should probably rather be slugified to abc-1
-    // otherwise we end up with 2 x "abc" anchors
-    // not sure how to implement that atm
     const expected = `
 
 # Ignored title
 
-## abc {#abc}
+## abc {#abc-1}
 
 ### Hello world {#hello-world}
 

packages/docusaurus/src/commands/writeHeadingIds.ts

@@ -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(/^-+/, '')
-    .replace(/-+$/, '');
+  const slug = slugger.slug(unwrapMarkdownLinks(headingText).trim(), {
+    maintainCase,
+  });
 
   return `${headingHashes}${headingText} {#${slug}}`;
 }
 
-export function transformMarkdownHeadingLine(
-  line: string,
-  slugger: Slugger,
+export function transformMarkdownContent(
+  content: string,
   options: Options = {maintainCase: false, overwrite: false},
 ): string {
   const {maintainCase = false, overwrite = false} = options;
-  if (!line.startsWith('#')) {
-    throw new Error(`Line is not a Markdown heading: ${line}.`);
-  }
-
-  const parsedHeading = parseMarkdownHeadingId(line);
+  const lines = content.split('\n');
+  const slugger = createSlugger();
 
-  // Do not process if id is already there
-  if (parsedHeading.id && !overwrite) {
-    return line;
+  // 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);
+      }
+    });
   }
-  return addHeadingId(parsedHeading.text, slugger, maintainCase);
-}
 
-function transformMarkdownLine(
-  line: string,
-  slugger: Slugger,
-  options?: Options,
-): string {
-  // Ignore h1 headings on purpose, as we don't create anchor links for those
-  if (line.startsWith('##')) {
-    return transformMarkdownHeadingLine(line, slugger, options);
-  }
-  return line;
-}
-
-function transformMarkdownLines(lines: string[], options?: Options): string[] {
   let inCode = false;
-  const slugger = createSlugger();
-
-  return lines.map((line) => {
-    if (line.startsWith('```')) {
-      inCode = !inCode;
-      return line;
-    }
-    if (inCode) {
-      return line;
-    }
-    return transformMarkdownLine(line, slugger, options);
-  });
-}
-
-export function transformMarkdownContent(
-  content: string,
-  options?: Options,
-): string {
-  return transformMarkdownLines(content.split('\n'), options).join('\n');
+  return lines
+    .map((line) => {
+      if (line.startsWith('```')) {
+        inCode = !inCode;
+        return line;
+      }
+      // Ignore h1 headings, as we don't create anchor links for those
+      if (inCode || !line.startsWith('##')) {
+        return line;
+      }
+      const parsedHeading = parseMarkdownHeadingId(line);
+
+      // Do not process if id is already there
+      if (parsedHeading.id && !overwrite) {
+        return line;
+      }
+      return addHeadingId(parsedHeading.text, slugger, maintainCase);
+    })
+    .join('\n');
 }
 
 async function transformMarkdownFile(
   filepath: string,
   options?: Options,
 ): Promise<string | undefined> {
   const content = await fs.readFile(filepath, 'utf8');
-  const updatedContent = transformMarkdownLines(
-    content.split('\n'),
-    options,
-  ).join('\n');
+  const updatedContent = transformMarkdownContent(content, options);
   if (content !== updatedContent) {
     await fs.writeFile(filepath, updatedContent);
     return filepath;