-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat(md): support mermaid #7544
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
PR Overview
This PR adds support for mermaid diagrams in MDX content and enhances the design system with a corresponding Storybook story.
- Implements a new Storybook story for mermaid diagrams.
- Integrates and enforces plugin ordering for the rehypeMermaid into the MDX pipeline.
Reviewed Changes
File | Description |
---|---|
apps/site/components/design/mermaid.stories.tsx | Adds a Storybook story to render mermaid diagrams via MDX compilation. |
apps/site/next.mdx.plugins.mjs | Inserts the rehypeMermaid plugin with explicit ordering instructions. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Issue here, is that we need playwright to render the diagram in back-end. So what do you think about this aproche. |
I mean there is also #7395, so this could help with both things |
FYI @AugustinMauroy build is failing:
|
yeah I know it's maybe a upstream issue. |
That ain't a valid reasoning. Please investigate :) |
yeah I mean I'm on it |
Okay update on this issue it's because next doesn't understand
|
It is not a Next.js problem. More like a SWC issue. It understands what |
Okay, here's the current status:
|
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 don't think it's practical to add tens of thousands of lines of code just to support Mermaid. Maintaining all these additions ourselves wouldn't be realistic.
Instead, such changes should be handled through upstream PRs or forks.
Okay let's me explain why this choice. |
I think that we should either open or monitor an issue on Next.js, and put mermaid on hold until that is resolved. I just can't realistically see us maintaining such a large (and not written by us) codebase, even if it's only temporary |
Already done !
"Huge" without test and fixture the patched package took 3 files |
On a different note, I'm not too keen on supporting Mermaid this way, as it prevents the website from being "out-of-the-box" buildable. Users would need to have a Playwright browser installed on their machine (right?), which, if I recall correctly, can take up to a gigabyte of space. That said, I submitted a PR to |
Sadly you need a proper DOM environment to render Mermaid diagrams. I also considered JSDom, but that won’t work, because it doesn’t support node dimensions. Another approach would be to use the |
The issue with that is Next.js actually try to bundle everything include if you specify |
That’s true. Webpack will still import and process export function createMermaidRenderer() {
// This is an empty stub
} And alias I don’t think this is ideal, but it would at least enable you to use Mermaid diagrams. My personal recommendation as an MDX maintainer would be to replace all custom MDX compilation logic with |
Okay. In that case, disregard what I said about playwright–i guess it's required
In my opinion, bundling a entire mermaid renderer in the client-side bundle is unnecessary and bloated for the client. Instead, we should use a package like the ones that @remcohaszing created.
Interesting, @ovflowd is this possible? @remcohaszing is there a way you can patch your library to not use That should resolve the issues we are facing, right? |
I really don’t see how.
So it really needs to resolve things on the file system somehow. |
Got it! Thanks again for your insight! I still feel that drastically increasing our codebase and using dependency overrides isn’t the best solution, so I’ll continue to keep this PR blocked for now. |
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.
If this increases our codebase drastically or the client bundle drastically, it is not worth it.
We should not bloat our bundles for something used in one blog post.
Edit by @avivkeller: Missing word
Unless there are any objections, I think this PR should be closed. |
I'll rebase don't worry |
6536c35
to
d997fff
Compare
d997fff
to
8793e20
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #7544 +/- ##
=======================================
Coverage 74.83% 74.83%
=======================================
Files 98 98
Lines 7888 7888
Branches 200 200
=======================================
Hits 5903 5903
Misses 1984 1984
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Rebase + removed "hosted" fork |
blocked by upstream |
Description
Support mermaid and add stories
Related Issues
close #7540
Check List
npm run format
to ensure the code follows the style guide.npm run test
to check if all tests are passing.npx turbo build
to check if the website builds without errors.