Skip to content
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

Throw error if mermaid rendering fails #321

Conversation

aloisklink
Copy link
Member

@aloisklink aloisklink commented Jun 26, 2022

📑 Summary

Throws an error if mermaid rendering fails. If there is an error:

  • mermaid-cli exits with exitcode 1.
  • The errors are logged to stderr (console.error).
  • Changed Behaviour: If there is an error, there is no output file. Previously there was an output file that said "Syntax error in graph: mermaid version 9.1.2" (e.g.: mermaid-error.png)

Resolves #276, resolves #245, resolves #138, resolves #109

Example:

me@me:~/mermaid-cli$ node index.bundle.js -i test-negative/invalid.expect-error.mmd -o test-output/invalid.expect-error.svg -c test-positive/config.json
Generating single mermaid chart

Parse error on line 2:
...nceDiagram  Nothing:Valid
----------------------^
Expecting 'SOLID_OPEN_ARROW', 'DOTTED_OPEN_ARROW', 'SOLID_ARROW', 'DOTTED_ARROW', 'SOLID_CROSS', 'DOTTED_CROSS', 'SOLID_POINT', 'DOTTED_POINT', got 'TXT'
me@me:~/mermaid-cli$ echo $?
1

📏 Design Decisions

Uses the new initThrowsError from Mermaid that was added in mermaid-js/mermaid#3052 by @MindaugasLaganeckas.

Important: I'm not sure whether to classify this as a feature or a fix.
Because this changes behaviour, maybe it is worth hiding this behind an option?
Or maybe we could add an option to use the old behaviour, something like --ignore-errors?

We should also add a warning to the release notes, e.g. something like

**⚠️ mermaid-cli now throws an error if mermaid fails, instead of making a diagram containing "Syntax Error"**

To simplify the src-test code, I used require("fs/promises");, which is only supported in NodeJS 14+. I don't think this is a big deal, since it's only in the test code, but I can change it to require("fs").promises (NodeJS 10+).

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
    • I tried to follow the old code style, e.g. I used "string" instead of 'string' when the old code used ". The style errors can be fixed with npx standard --fix though.
  • 💻 have added unit/e2e tests (if appropriate)
    • The test code now runs on the test-negative folder too.
  • 🔖 targeted master branch

9.1.2 is the minimum required version that exposes
initThrowsErrors to the client, see
https://github.com/mermaid-js/mermaid/releases/tag/9.1.2
Previously, any errors when testing stdin would be
encoded as 'buffer', not 'utf8.
Currently, piping markdown through stdin is not supported,
as mermaid-cli has no idea it's markdown, not mermaid code.
We use `window.mermaid.initThrowsErrors` to throw an error if mermaid
rendering fails.

Previously, if mermaid failed to render, it would output a generic
SVG/.png that said that rendering failed.

BREAKING CHANGE: Mermaid now throws an error if rendering fails,
                 instead of just outputing an SVG that said that
                 rendering failed.
Fixes style issues that cannot be automatically fixed
by running `standard --fix`.

I've purposely not run `standard --fix`, in order to keep
any `git diff` as small as possible.
Previously, only the `test-positive` workflow folder
was tested.

Additionally, when an `expectError` test fails,
we log: `✅ compiling ${file} produced an error, which is well`,
as otherwise it can be a bit confusing to view the logs.
Previously, any errors found by running mmdc
were output to stdout using console.log,
when they should have instead been logged to
console.error.
@@ -1,25 +1,27 @@
"use strict";

const fs = require("fs");
const fs = require("fs/promises");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using require("fs/promises") here because it is what is recommended by the Node.JS documentation: see https://nodejs.org/docs/latest-v18.x/api/fs.html#promises-api

However, if we want to support Node.JS v10 and v12 (possibly not needed, since this is just test code), we can change to:

Suggested change
const fs = require("fs/promises");
const fs = require("fs").promises;

@MindaugasLaganeckas
Copy link
Member

Thank you for helping me out!!! 🏅 😄 This has been a long awaited feature!

@MindaugasLaganeckas MindaugasLaganeckas merged commit c811fd9 into mermaid-js:master Jun 27, 2022
@aloisklink
Copy link
Member Author

Thank you for helping me out!!! 🏅 😄 This has been a long awaited feature!

No worries 😄, I've got a library that uses mermaid-cli, so I was happy to help!

@aloisklink aloisklink deleted the feature/throw-error-on-mermaid-error branch June 27, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants