-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
fix: make mermaid.parse
throw an error on invalid shapes
#6002
fix: make mermaid.parse
throw an error on invalid shapes
#6002
Conversation
I went through `packages/mermaid/src/diagrams/flowchart/parser/flow.jison` and found every possible value this `type` parameter could be.
Currently, invalid shapes cause an error when rendering, but not when parsing. This confuses the Mermaid Live Editor, e.g. by not showing the error message. Instead, I've added an `isValidShape()` that validates if the shape is valid at parse time. This only checks shapes using the new extended shapes syntax. Currenlty, using `A(-this is an ellipse node-)` is broken (see mermaid-js#5976) and also causes an invalid shape error at render time, but I've ignored it in this PR, so it will continue pass at parse-time (we have unit tests checking ellipse parsing). See: mermaid-js#5976
🦋 Changeset detectedLatest commit: f6c32b6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6002 +/- ##
==========================================
- Coverage 4.50% 4.50% -0.01%
==========================================
Files 384 383 -1
Lines 53867 53873 +6
Branches 596 622 +26
==========================================
Hits 2425 2425
- Misses 51442 51448 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM!
📑 Summary
Currently, invalid shapes cause an error when rendering, but not when parsing. This confuses the Mermaid Live Editor, e.g. by not showing the error message.
Instead, I've added an
isValidShape()
that validates if the shape is valid at parse time. This only checks shapes using the new extended shapes syntax.E.g.
Example:
See mermaid.live
As you can see, mermaid.live doesn't show an error message, even though the diagram failed to render:
📏 Design Decisions
I've used TypeScript to ensure that all the types are correct, with one issue:
Currently, using
A(-this is an ellipse node-)
is broken (see #5976) and also causes an invalid shape error at render time, but I've ignored it in this PR, so it will continue pass at parse-time (we have unit tests checking ellipse parsing).The list of valid types for
FlowVertex
are all taken from:FlowVertexTypeParam
:mermaid/packages/mermaid/src/diagrams/flowchart/parser/flow.jison
Lines 428 to 458 in e765007
ShapeID
type:mermaid/packages/mermaid/src/rendering-util/rendering-elements/shapes.ts
Line 493 in e765007
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.