Skip to content

Commit

Permalink
fix: error mermaid.parse on invalid shapes
Browse files Browse the repository at this point in the history
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
  • Loading branch information
aloisklink committed Oct 18, 2024
1 parent adae243 commit 436b4c0
Show file tree
Hide file tree
Showing 7 changed files with 42 additions and 10 deletions.
5 changes: 5 additions & 0 deletions .changeset/thick-elephants-search.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'mermaid': patch
---

fix: error `mermaid.parse` on an invalid shape, so that it matches the errors thrown by `mermaid.render`
19 changes: 12 additions & 7 deletions packages/mermaid/src/diagrams/flowchart/flowDb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { select } from 'd3';
import utils, { getEdgeId } from '../../utils.js';
import { getConfig, defaultConfig } from '../../diagram-api/diagramAPI.js';
import common from '../common/common.js';
import { isValidShape, type ShapeID } from '../../rendering-util/rendering-elements/shapes.js';
import type { Node, Edge } from '../../rendering-util/types.js';
import { log } from '../../logger.js';
import * as yaml from 'js-yaml';
Expand Down Expand Up @@ -140,14 +141,15 @@ export const addVertex = function (
}
// console.log('yamlData', yamlData);
const doc = yaml.load(yamlData, { schema: yaml.JSON_SCHEMA }) as NodeMetaData;
if (doc.shape && (doc.shape !== doc.shape.toLowerCase() || doc.shape.includes('_'))) {
throw new Error(`No such shape: ${doc.shape}. Shape names should be lowercase.`);
}

// console.log('yamlData doc', doc);
if (doc?.shape) {
if (doc.shape) {
if (doc.shape !== doc.shape.toLowerCase() || doc.shape.includes('_')) {
throw new Error(`No such shape: ${doc.shape}. Shape names should be lowercase.`);
} else if (!isValidShape(doc.shape)) {
throw new Error(`No such shape: ${doc.shape}.`);
}
vertex.type = doc?.shape;
}

if (doc?.label) {
vertex.text = doc?.label;
}
Expand Down Expand Up @@ -823,7 +825,7 @@ export const lex = {
firstGraph,
};

const getTypeFromVertex = (vertex: FlowVertex) => {
const getTypeFromVertex = (vertex: FlowVertex): ShapeID => {
if (vertex.img) {
return 'imageSquare';
}
Expand All @@ -845,6 +847,9 @@ const getTypeFromVertex = (vertex: FlowVertex) => {
return 'squareRect';
case 'round':
return 'roundedRect';
case 'ellipse':
// @ts-expect-error -- Ellipses are broken, see https://github.com/mermaid-js/mermaid/issues/5976
return 'ellipse';
default:
return vertex.type;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,21 @@ describe('when parsing directions', function () {
expect(data4Layout.nodes[0].shape).toEqual('squareRect');
expect(data4Layout.nodes[0].label).toEqual('This is }');
});
it('should error on non-existent shape', function () {
expect(() => {
flow.parser.parse(`flowchart TB
A@{ shape: this-shape-does-not-exist }
`);
}).toThrow('No such shape: this-shape-does-not-exist.');
});
it('should error on internal-only shape', function () {
expect(() => {
// this shape does exist, but it's only supposed to be for internal/backwards compatibility use
flow.parser.parse(`flowchart TB
A@{ shape: rect_left_inv_arrow }
`);
}).toThrow('No such shape: rect_left_inv_arrow. Shape names should be lowercase.');
});
it('Diamond shapes should work as usual', function () {
const res = flow.parser.parse(`flowchart TB
A{This is a label}
Expand Down
4 changes: 3 additions & 1 deletion packages/mermaid/src/diagrams/flowchart/types.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { ShapeID } from '../../rendering-util/rendering-elements/shapes.js';

/**
* Valid `type` args to `yy.addVertex` taken from
* `packages/mermaid/src/diagrams/flowchart/parser/flow.jison`
Expand Down Expand Up @@ -33,7 +35,7 @@ export interface FlowVertex {
props?: any;
styles: string[];
text?: string;
type?: string;
type?: ShapeID | FlowVertexTypeParam;
icon?: string;
form?: string;
pos?: 't' | 'b';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export async function insertNode(
}
}

const shapeHandler = shapes[(node.shape ?? 'undefined') as keyof typeof shapes];
const shapeHandler = node.shape ? shapes[node.shape] : undefined;

if (!shapeHandler) {
throw new Error(`No such shape: ${node.shape}. Please check your syntax.`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -491,4 +491,8 @@ const generateShapeMap = () => {

export const shapes = generateShapeMap();

export function isValidShape(shape: string): shape is ShapeID {
return shape in shapes;
}

export type ShapeID = keyof typeof shapes;
3 changes: 2 additions & 1 deletion packages/mermaid/src/rendering-util/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export type MarkdownWordType = 'normal' | 'strong' | 'em';
import type { MermaidConfig } from '../config.type.js';
import type { ShapeID } from './rendering-elements/shapes.js';
export interface MarkdownWord {
content: string;
type: MarkdownWordType;
Expand Down Expand Up @@ -37,7 +38,7 @@ export interface Node {
linkTarget?: string;
tooltip?: string;
padding?: number; //REMOVE?, use from LayoutData.config - Keep, this could be shape specific
shape?: string;
shape?: ShapeID;
isGroup: boolean;
width?: number;
height?: number;
Expand Down

0 comments on commit 436b4c0

Please sign in to comment.