-
-
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: support mermaid code blocks in Markdown #7490
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
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.
We need to make this opt-in.
Before we allow plugins to provide Markdown plugins (#6370), what about keeping the MDX loader part but hiding it behind a siteConfig.markdown.mermaid
option that defaults to false, and also create another @docusaurus/theme-mermaid
package for the theme components?
I shall have a look at making those 2 changes. Should I move the mermaid and theme config into Something like: export type DocusaurusConfig = {
markdown?: {
mermaid?: boolean | {
theme?: {
light: mermaidAPI.Theme;
dark: mermaidAPI.Theme;
[htmlTheme: string]: mermaidAPI.Theme;
};
config?: mermaidAPI.Config;
}
}
} |
Theme options should be still in theme config. The |
packages/docusaurus-theme-classic/src/theme/MDXComponents/index.tsx
Outdated
Show resolved
Hide resolved
packages/docusaurus-theme-classic/src/theme/MDXComponents/index.tsx
Outdated
Show resolved
Hide resolved
Sorry, left a bunch of random review comments... There's a bit left to be desired in this PR's approach. Let me know if you need help with any of the refactors. We are not in a rush anyways. |
No worries, I wasn't at all familair with how Swizzle works before this PR so there was bound to be teething problems :) Hopefully now all good 🤞 |
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, this is starting to look good!
I don't have much feedback for you to change. I'll play with this myself over the following days and push to your branch.
// Assign a unique ID to each mermaid svg as per requirements of | ||
// `mermaid.render`. |
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 looks very unsafe to have every effect mutate something... Effects aren't guaranteed to run in any order or run at any particular time, so this is practically not much better than assigning a random number to each SVG. This should be fixable with React 18's useId
hook?
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 think so, shouldn't be an issue for now though 🤞
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.
All diagrams on the same page will have the exact same ID because the counter is distinct for each instance.
I don't understand why Mermaid want an ID, can you explain?
If it's only used client-side, you could just wrap the component in <BrowserOnly>
and generate a random number for each instance? (ie no need to increment)
…tribute markdownConfig
Not even documented: mermaid.render returns the svg string | ||
Using the documented form is un-necessary | ||
*/ | ||
return mermaid.render(mermaidId, txt); |
Check warning
Code scanning / CodeQL
Unsafe HTML constructed from library input
Note for future self: I'm removing the import MDXComponents from '@theme-init/MDXComponents';
import Mermaid from '@theme/Mermaid';
export default {
...MDXComponents,
mermaid: Mermaid,
}; We want the mermaid theme to be able to register a theme component to the MDX context so that mdx loader can render it. Unfortunately, we want a plugin lifecycle API to allow it, but we don't have it yet. The code above is not ideal because we can only wrap and "enhance" MDXComponents once, cf my comment (#7490 (comment)) and #4530, so I'd prefer not to use this solution. Instead I'm doing like the SearchBar: render nothing by default in the theme classic, and let the theme enhancer override the implementation. Having a NOOP Mermaid component in theme-classic allows it to register it automatically to the MDXComponents so that we don't need to enhance this map |
Note: this implementation does not support server-side-rendering and has a few limitations:
Unfortunately Mermaid does not make it easy to pre-render SVGs at build time (see mermaid-js/mermaid#3650 (comment)). It would require to run a headless browser like Puppeteer, and for dark/light mode support we would have to build each diagram twice, which is also not ideal. For now let's keep things simple and only render client-side with the known limitations. |
Wooooooooo 🎉 |
Co-authored-by: Joshua Chen <sidachen2003@gmail.com> Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com> # Conflicts: # project-words.txt # website/package.json # yarn.lock
Pre-flight checklist
Note for third-party plugin authors using mdx loader
You should pass this new mdx loader option:
Motivation
#1258 Have Mermaid diagrams supported OOTB. Merged code from mdx-mermaid library
Test Plan
There are unit tests for the remark plugin. There are also diagram examples from the Mermaid docs in the dogfooding pages
For the config here is an example
docusaurus.config.js
themeConfig
:Test links
Deploy preview:
Local:
Related issues/PRs