-
Notifications
You must be signed in to change notification settings - Fork 15
fix(toc): prevent misnamed mdx components from breaking TOC #1242
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,4 +175,40 @@ export const toc = [ | |
| </ul></li></ul></nav>" | ||
| `); | ||
| }); | ||
|
|
||
| it('preserves nesting when component names differ from exported elements', () => { | ||
| const md = ` | ||
| # Title | ||
|
|
||
| ## SubHeading | ||
|
|
||
| <Comp> | ||
| First | ||
| </Comp> | ||
| `; | ||
|
|
||
| const components = { | ||
| Comp: 'export const Comp = ({ children }) => { return children; }', | ||
| }; | ||
|
|
||
| const compModule = run(compile(components.Comp)); | ||
| const { Toc } = run(compile(md, { components }), { | ||
| components: { | ||
| // 👇🏼 this is what we're guarding against | ||
| CompDoesNotMatchExportedModule: compModule, | ||
| }, | ||
| }); | ||
|
|
||
| const html = renderToString(<Toc />); | ||
| expect(html).toMatchInlineSnapshot(` | ||
| "<nav aria-label="Table of contents" role="navigation"><ul class="toc-list"><li><a class="tocHeader" href="#"><i class="icon icon-text-align-left"></i>Table of Contents</a></li><li class="toc-children"><ul> | ||
| <li><a href="#title">Title</a></li> | ||
| <li> | ||
| <ul> | ||
| <li><a href="#subheading">SubHeading</a></li> | ||
| </ul> | ||
|
Comment on lines
+207
to
+209
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when everything works properly, the subheading should be indented one level like this |
||
| </li> | ||
| </ul></li></ul></nav>" | ||
| `); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,14 +58,31 @@ export const rehypeToc = ({ components = {} }: Options): Transformer<Root, Root> | |
| }; | ||
|
|
||
| const MAX_DEPTH = 2; | ||
| const getDepth = (el: HastHeading) => parseInt(el.tagName?.match(/^h(\d)/)[1], 10); | ||
|
|
||
| /** | ||
| * Get the depth of a heading element based on its tag name. | ||
| * | ||
| * ⚠️ Be extra defensive to non-heading elements somehow making it here. This | ||
| * should not happen, but if it does, we avoid breaking TOC depth calculations | ||
| * by returning `Infinity`, thus removing them from depth considerations. | ||
| * @link https://linear.app/readme-io/issue/CX-2543/tabapay-toc-does-not-respect-headers-nesting | ||
| * | ||
| * @example | ||
| * getDepth({ tagName: 'h1' }) // 1 | ||
| * getDepth({ tagName: 'h2' }) // 2 | ||
| */ | ||
| const getDepth = (el: HastHeading) => { | ||
| if (!el.tagName) return Infinity; | ||
| return parseInt(el.tagName?.match(/^h(\d)/)[1], 10); | ||
| }; | ||
|
|
||
| /* | ||
| * `tocToHast` consumes the list generated by `rehypeToc` and produces a hast | ||
| * of nested lists to be rendered as a table of contents. | ||
| */ | ||
| const tocToHast = (headings: HastHeading[] = []): TocList => { | ||
| const min = Math.min(...headings.map(getDepth)); | ||
| const headingDepths = headings.map(getDepth); | ||
| const min = Math.min(...headingDepths); | ||
| const ast = h('ul') as TocList; | ||
| const stack: TocList[] = [ast]; | ||
|
|
||
|
|
@@ -105,7 +122,10 @@ export const tocHastToMdx = (toc: IndexableElements[] | undefined, components: R | |
| if (typeof toc === 'undefined') return ''; | ||
|
|
||
| const injected = toc.flatMap(node => { | ||
| return node.type === 'mdxJsxFlowElement' && node.name in components ? components[node.name] || [] : node; | ||
| if (node.type === 'mdxJsxFlowElement') { | ||
| return components[node.name] || []; | ||
| } | ||
|
Comment on lines
+125
to
+127
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the exported JSX elements are the nodes (i.e. now, if the node type is an mdx jsx element, we'll return an empty array instead, which essentially filters it out completely. i think this is the desired result, but i put in a patch in the |
||
| return node; | ||
| }); | ||
|
|
||
| const tocHast = tocToHast(injected as HastHeading[]); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the edge-case that was causing the malformed TOCs in some projects.
when they have custom components that exports a JSX element that differs from the saved file name, this is what happens. the components hash keys are something different than the exported elements.