🧜♀️ Create images of mermaid diagrams for static exports #2132
Conversation
🦋 Changeset detectedLatest commit: 916724a The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
rowanc1
changed the title
| describe('Download Template', { timeout: 15000 }, () => { | ||
| it('Download default template', async () => { |
There was a problem hiding this comment.
This is just putting the timeout as the second arg.
| ```bash | ||
| # Automatic detection (recommended for CI) | ||
| CI=true npm run build | ||
|
|
||
| # Manual control | ||
| MERMAID_NO_SANDBOX=true npm run build | ||
| ``` |
There was a problem hiding this comment.
CI was way harder to get going than I thought. Should be automatic now though!
There was a problem hiding this comment.
Tested this out - it works very smoothly! First I did not have mmdc installed - my diagram rendered with myst start but the PDF export reported an error with install instructions. Then I installed mmdc and tried the export again and the diagram conversion worked:
My only requested change is removing @mermaid-js/mermaid-cli from our dependencies; to me, it feels heavy and unnecessary - it's easy for users to install the CLI separately.
Other than that - there's a few high-level aspects that gave me pause (new transforms API for processMdast functions; different way of finalizing images for "static" output outside of finalizeMdast function). I made some comments and am happy to discuss further, but I don't think those discussions need to block landing this feature.
| }, | ||
| "dependencies": { | ||
| "@jupyterlab/services": "^7.0.0", | ||
| "@mermaid-js/mermaid-cli": "^10.9.1", |
There was a problem hiding this comment.
I think this dependency is only here to install mmdc CLI, not ever imported in the codebase. Is that correct?
If so, I think we should remove it as a dependency for all users. We already have installation instructions in the error message when mermaid conversion fails.
There was a problem hiding this comment.
How are dependencies determined for the overall mystmd package? Presumably, you could have mmdc as a runtime dependency?
| imageExtensions?: ImageExtensions[]; | ||
| watchMode?: boolean; | ||
| execute?: boolean; | ||
| transforms?: TransformSpec[]; |
There was a problem hiding this comment.
This small change is actually a pretty substantial change to the API - It's a completely new way to specify transforms within the code. I think it is non-controversial and for-the-better: matching how external plugins are defined and executed is great.
But worth noting this alone is a new feature, entirely independent of the mermaid stuff.
| projectPath, | ||
| imageExtensions: DOCX_IMAGE_EXTENSIONS, | ||
| extraLinkTransformers, | ||
| transforms: [createMermaidImageMystPlugin], |
There was a problem hiding this comment.
Injecting this as transforms for each export type is interesting.
The current pattern for "we need to transform something so it works in a static export" is put it in finalizeMdast under the flag simplifyFigures: true, e.g. https://github.com/jupyter-book/mystmd/blob/main/packages/myst-cli/src/process/mdast.ts#L441
Using a transform instead is maybe (probably...?) better: the interface is cleaner and follows existing plugin work, and we are not just dumping more random stuff in the finalizeMdast function. However, this now splits transforms with similar purpose ("simplify images for print") across 2 ways of doing things.
Probably fine to leave as-is, and maybe start moving the finalizeMdast steps to transforms (although that's not trivial - many of those steps need to happen as the very last thing during processing and right now transforms are only run at specific, earlier points). (Also... that begs the question - should the mermaid transform be run later during processing? Probably not...? But there could be edge cases where a mermaid transform is added later and never converted to an image.)
|
|
||
| :::{note .dropdown} For CI Environments | ||
|
|
||
| The Mermaid CLI uses Puppeteer which may require special configuration in CI environments. MyST automatically handles this by detecting the `CI` environment variable or you can manually control it: |
There was a problem hiding this comment.
What is being handled here?
The example is a bit confusing, because CI is already set on both GH and GitLab, so user would never set it themselves.
| ```bash | ||
| # Automatic detection (recommended for CI) | ||
| CI=true npm run build | ||
|
|
||
| # Manual control | ||
| MERMAID_NO_SANDBOX=true npm run build | ||
| ``` |
| MERMAID_NO_SANDBOX=true npm run build | ||
| ``` | ||
|
|
||
| This automatically creates a Puppeteer configuration file with `--no-sandbox` flag to resolve sandbox issues in Linux CI environments. |
There was a problem hiding this comment.
OK, this answers part of my question from above. Does generating the config equate to disabling the sandbox? If so, maybe something like:
By default, Mermaid runs Puppeteer in a sandbox. However, in CI it needs to run directly in the current session. When the CI environment variable is set (which is the case for GitHub and GitLab), then myst will disable the sandbox by generating a suitable Puppeteer configuration file. You can also control this behavior explicitly with `MERMAID_DISABLE_SANDBOX=true/false`.
X_NO_Y namings don't read correctly in my head, but could be just me. Should this be PUPPETEER_DISABLE_SANDBOX or MERMAID_DISABLE_SANDBOX?
|
|
||
| expect(result.type).toBe('image'); | ||
| if (result.type === 'image') { | ||
| expect(result.url).toMatch(/^data:image\/svg\+xml;base64,/); |
| }, | ||
| "dependencies": { | ||
| "@jupyterlab/services": "^7.0.0", | ||
| "@mermaid-js/mermaid-cli": "^10.9.1", |
There was a problem hiding this comment.
How are dependencies determined for the overall mystmd package? Presumably, you could have mmdc as a runtime dependency?
| const outputFile = path.join(tempFolder, `mermaid-output-${hash}.${format}`); | ||
|
|
||
| if (!isMermaidCliAvailable()) { | ||
| throw new Error('Mermaid CLI is not available'); |
There was a problem hiding this comment.
| throw new Error('Mermaid CLI is not available'); | |
| throw new Error('Mermaid CLI is not available: try installing using `npm install -g @mermaid-js/mermaid-cli`'); |
If not this, then point users to the docs page where it is discussed?
| try { | ||
| // Create puppeteer config file for CI environments | ||
| // See https://github.com/mermaid-js/mermaid-cli/blob/master/docs/linux-sandbox-issue.md | ||
| const useNoSandbox = process.env.MERMAID_NO_SANDBOX === 'true' || process.env.CI === 'true'; |
There was a problem hiding this comment.
In the docs, it says the user gets to control the behavior. It sounded like "regardless of whether in CI or not". So, what happens when MERMAID_NO_SANDBOX === 'false' and CI === true? Maybe the condition should be:
const useNoSandbox =
process.env.MERMAID_NO_SANDBOX !== undefined
? process.env.MERMAID_NO_SANDBOX === 'true'
: process.env.CI === 'true';
|
|
||
| // Render using Mermaid CLI with stdin input | ||
| await execAsync( | ||
| `echo '${mermaidCode.replace(/'/g, "'\"'\"'")}' | mmdc -i - -o "${outputFile}" -t ${theme} -b transparent${puppeteerConfigFlag}`, |
There was a problem hiding this comment.
Yikes, this shell escaping is hard to parse.
Perhaps we need a utility function that promisifies spawn and accepts standard input, a-la https://stackoverflow.com/a/72863312?
There was a problem hiding this comment.
Also, is the escaping of -o sufficient for strange filenames?
| ): Promise<string> { | ||
| // Create unique temporary output file name | ||
| const hash = crypto.createHash('md5').update(mermaidCode).digest('hex'); | ||
| const tempFolder = createTempFolder(); |
There was a problem hiding this comment.
Would be nice to have these clean up after themselves, like in Python. Using something like https://github.com/Mcsavvy/contextlib or a custom implementation:
withTempDir(callback)
| fileError(file, message, { | ||
| ruleId: RuleId.imageFormatConverts, | ||
| node, | ||
| note: 'To install the Mermaid CLI, run `npm install -g @mermaid-js/mermaid-cli`', |
There was a problem hiding this comment.
Is this the correct error here? We already throw above if CLI not found.
Fixes #2006
Requires that you have
mmdcinstalled, the CLI warns you and I have added some docs. Works with typst, tex, docx and jats.Replaces #1452