-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
feat(mdx-loader): the table-of-contents should display toc/headings of imported MDX partials #9684
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
I consider this PR done. |
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.
That looks like good start thanks 👍
I'm not 100% fan of the implementation yet but overall the solution looks like what we want to adopt.
I'd like to have much more unit tests, testing more complex cases, such as using the same partial twice in a page, or importing the same partial using different names.
That would also be interesting to dogfood with more complex cases, such as a deeply nested tree of partials (having partials importing other partials)
id: 'thanos', | ||
level: 2 | ||
}, | ||
...toc0 |
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.
toc0 is not imported here, is this expected?
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.
Seems like we're going down the road of only modifying the AST and not the actual js, so this is expected.
id: child.data!.id!, | ||
level: child.depth, | ||
}); | ||
if (child.type === 'mdxjsEsm') { |
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.
I know the original code was already low-level, but I'd appreciate if we could make it more readable and high-level. I mean the intent of this algorithm should be clear without spending time trying to understand the implementation details. Currently it's a bit hard to see the big picture of it. I would extract things in smaller functions for example.
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.
Moved the visitor for each type of node into its own function. This is a bit more readable now, but I can try to do some further reorganisation if needed.
Thanks for the detailed feedback! I'll be working on improving this PR in the following weeks. |
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.
I'm quite happy with this implementation; exactly how I pictured it.
Co-authored-by: Titus <tituswormer@gmail.com>
Moved the dogfooding into Also since I changed the plugin to not generate any js anymore, only ASTs, the existing unit tests for the ToC plugin became useless, as they checked generated mdx. I changed the unit tests to output ASTs, so the work of the plugin is visible. |
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.
Last one is not working, the heading shows the text passed to the prop as expected, but the ToC displays just the text props.propname.
What output do you want, what would be “working”?
The code you use uses toValue
: https://github.com/anatolykopyl/docusaurus/blob/1fdd6af8c79e7765ff4d7b4731e605d0d95c1316/packages/docusaurus-mdx-loader/src/remark/utils/index.ts#L76C17-L76C24.
Which calls toString()
on an expression ({props.propname}
): https://github.com/anatolykopyl/docusaurus/blob/1fdd6af8c79e7765ff4d7b4731e605d0d95c1316/packages/docusaurus-mdx-loader/src/remark/utils/index.ts#L99
Which returns node.value
, which is props.propname
.
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.
Thanks, that looks like it's working nicely.
The {props.propname}
is not a problem to consider for now so you can remove that test.
I understand why you changed the test snapshots to MDAST although I don't like it. I'll try to come up with a solution and commit a thing to your PR soon.
packages/docusaurus-mdx-loader/src/remark/toc/__tests__/index.test.ts
Outdated
Show resolved
Hide resolved
website/_dogfooding/_docs tests/toc-partials/_partial-with-props.mdx
Outdated
Show resolved
Hide resolved
Thanks @anatolykopyl , looks like it's working nicely. Almost ready, I'm just going to refactor some details and merge asap! |
Note: @anatolykopyl started this PR, and then @slorber took it over.
Motivation
Fix #3915
It is now possible to import MDX partials and have headings of the partial appear in the TOC.
We'll consider this as a "new feature" instead of a bug fix, considering it remains an important behavior change, and an improvement over stock MDX. We'll release this in v3.2 instead of v3.1.1
Consider the following:
This doc now renders a TOC with 2 headings, while previously "Partial heading" didn't appear in the TOC.
In practice, the doc is now compiled as:
Where
__tocPartial
contains{"value": "Partial heading","id": "partial-heading","level": 2}
This makes it possible to have headings working for "deeply-nested-partials" where one partial import another.
Notable change: if you write
export const toc = ...
in your MDX docs, we now never override this value anymore. Previously we did, but only when there were headings. We consider this as a bug fix and not a breaking change.Test Plan
Unit tests + dogfood
Test links
https://deploy-preview-9684--docusaurus-2.netlify.app/tests/pages/partials-tests
Related issues/PRs
This PR adapts a similar fix from @dimaMachina made in Nextra to Docusaurus: shuding/nextra#2199