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

fix: Detector order #4124

Merged
merged 5 commits into from
Feb 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/mermaid/src/diagram-api/detectType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,24 @@ export const detectType = function (text: string, config?: MermaidConfig): strin
}
}

throw new UnknownDiagramError(`No diagram type detected for text: ${text}`);
throw new UnknownDiagramError(
`No diagram type detected matching given configuration for text: ${text}`
);
};

/**
* Registers lazy-loaded diagrams to Mermaid.
*
* The diagram function is loaded asynchronously, so that diagrams are only loaded
* if the diagram is detected.
*
* @remarks
* Please note that the order of diagram detectors is important.
* The first detector to return `true` is the diagram that will be loaded
* and used, so put more specific detectors at the beginning!
*
* @param diagrams - Diagrams to lazy load, and their detectors, in order of importance.
*/
export const registerLazyLoadedDiagrams = (...diagrams: ExternalDiagramDefinition[]) => {
for (const { id, detector, loader } of diagrams) {
addDetector(id, detector, loader);
Expand Down
82 changes: 82 additions & 0 deletions packages/mermaid/src/diagram-api/diagram-orchestration.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { it, describe, expect } from 'vitest';
import { detectType } from './detectType';
import { addDiagrams } from './diagram-orchestration';

describe('diagram-orchestration', () => {
it('should register diagrams', () => {
expect(() => detectType('graph TD; A-->B')).toThrow();
addDiagrams();
expect(detectType('graph TD; A-->B')).toBe('flowchart');
});

describe('proper diagram types should be detetced', () => {
beforeAll(() => {
addDiagrams();
});

it.each([
{ text: 'graph TD;', expected: 'flowchart' },
{ text: 'flowchart TD;', expected: 'flowchart-v2' },
aloisklink marked this conversation as resolved.
Show resolved Hide resolved
{ text: 'flowchart-v2 TD;', expected: 'flowchart-v2' },
{ text: 'flowchart-elk TD;', expected: 'flowchart-elk' },
{ text: 'error', expected: 'error' },
{ text: 'C4Context;', expected: 'c4' },
{ text: 'classDiagram', expected: 'class' },
{ text: 'classDiagram-v2', expected: 'classDiagram' },
{ text: 'erDiagram', expected: 'er' },
{ text: 'journey', expected: 'journey' },
{ text: 'gantt', expected: 'gantt' },
{ text: 'pie', expected: 'pie' },
{ text: 'requirementDiagram', expected: 'requirement' },
{ text: 'info', expected: 'info' },
{ text: 'sequenceDiagram', expected: 'sequence' },
{ text: 'mindmap', expected: 'mindmap' },
{ text: 'timeline', expected: 'timeline' },
{ text: 'gitGraph', expected: 'gitGraph' },
{ text: 'stateDiagram', expected: 'state' },
{ text: 'stateDiagram-v2', expected: 'stateDiagram' },
])(
'should $text be detected as $expected',
({ text, expected }: { text: string; expected: string }) => {
expect(detectType(text)).toBe(expected);
}
);

it('should detect proper flowchart type based on config', () => {
// graph & dagre-d3 => flowchart
expect(detectType('graph TD; A-->B')).toBe('flowchart');
// graph & dagre-d3 => flowchart
expect(detectType('graph TD; A-->B', { flowchart: { defaultRenderer: 'dagre-d3' } })).toBe(
'flowchart'
);
// flowchart & dagre-d3 => error
expect(() =>
detectType('flowchart TD; A-->B', { flowchart: { defaultRenderer: 'dagre-d3' } })
).toThrowErrorMatchingInlineSnapshot(
'"No diagram type detected matching given configuration for text: flowchart TD; A-->B"'
);

// graph & dagre-wrapper => flowchart-v2
expect(
detectType('graph TD; A-->B', { flowchart: { defaultRenderer: 'dagre-wrapper' } })
).toBe('flowchart-v2');
// flowchart ==> flowchart-v2
expect(detectType('flowchart TD; A-->B')).toBe('flowchart-v2');
// flowchart && dagre-wrapper ==> flowchart-v2
expect(
detectType('flowchart TD; A-->B', { flowchart: { defaultRenderer: 'dagre-wrapper' } })
).toBe('flowchart-v2');
// flowchart && elk ==> flowchart-elk
expect(detectType('flowchart TD; A-->B', { flowchart: { defaultRenderer: 'elk' } })).toBe(
'flowchart-elk'
);
});

it('should not detect flowchart if pie contains flowchart', () => {
expect(
detectType(`pie title: "flowchart"
flowchart: 1 "pie" pie: 2 "pie"`)
).toBe('pie');
});
});
});
11 changes: 6 additions & 5 deletions packages/mermaid/src/diagram-api/diagram-orchestration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const addDiagrams = () => {
throw new Error(
'Diagrams beginning with --- are not valid. ' +
'If you were trying to use a YAML front-matter, please ensure that ' +
"you've correctly opened and closed the YAML front-matter with unindented `---` blocks"
"you've correctly opened and closed the YAML front-matter with un-indented `---` blocks"
);
},
},
Expand All @@ -55,25 +55,26 @@ export const addDiagrams = () => {
return text.toLowerCase().trimStart().startsWith('---');
}
);
// Ordering of detectors is important. The first one to return true will be used.
sidharthv96 marked this conversation as resolved.
Show resolved Hide resolved
registerLazyLoadedDiagrams(
error,
c4,
classDiagram,
classDiagramV2,
classDiagram,
er,
gantt,
info,
pie,
requirement,
sequence,
flowchart,
flowchartV2,
flowchartElk,
flowchartV2,
flowchart,
mindmap,
timeline,
git,
state,
stateV2,
state,
journey
);
};
4 changes: 2 additions & 2 deletions packages/mermaid/src/diagram-api/diagramAPI.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ describe('DiagramAPI', () => {

it('should handle diagram registrations', () => {
expect(() => getDiagram('loki')).toThrow();
expect(() => detectType('loki diagram')).toThrow(
'No diagram type detected for text: loki diagram'
expect(() => detectType('loki diagram')).toThrowErrorMatchingInlineSnapshot(
'"No diagram type detected matching given configuration for text: loki diagram"'
);
const detector: DiagramDetector = (str: string) => {
return str.match('loki') !== null;
Expand Down
4 changes: 2 additions & 2 deletions packages/mermaid/src/diagram.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ Expecting 'TXT', got 'NEWLINE'"
});

test('should throw the right error for unregistered diagrams', async () => {
await expect(getDiagramFromText('thor TD; A-->B')).rejects.toThrowError(
'No diagram type detected for text: thor TD; A-->B'
await expect(getDiagramFromText('thor TD; A-->B')).rejects.toThrowErrorMatchingInlineSnapshot(
'"No diagram type detected matching given configuration for text: thor TD; A-->B"'
);
});
});
10 changes: 5 additions & 5 deletions packages/mermaid/src/diagrams/flowchart/flowDetector-v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ import type { ExternalDiagramDefinition } from '../../diagram-api/types';
const id = 'flowchart-v2';

const detector: DiagramDetector = (txt, config) => {
if (config?.flowchart?.defaultRenderer === 'dagre-d3') {
return false;
}
if (config?.flowchart?.defaultRenderer === 'elk') {
if (
config?.flowchart?.defaultRenderer === 'dagre-d3' ||
config?.flowchart?.defaultRenderer === 'elk'
) {
return false;
}

// If we have configured to use dagre-wrapper then we should return true in this function for graph code thus making it use the new flowchart diagram
if (txt.match(/^\s*graph/) !== null) {
if (txt.match(/^\s*graph/) !== null && config?.flowchart?.defaultRenderer === 'dagre-wrapper') {
return true;
}
return txt.match(/^\s*flowchart/) !== null;
Expand Down
8 changes: 4 additions & 4 deletions packages/mermaid/src/diagrams/flowchart/flowDetector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ const id = 'flowchart';
const detector: DiagramDetector = (txt, config) => {
// If we have conferred to only use new flow charts this function should always return false
// as in not signalling true for a legacy flowchart
if (config?.flowchart?.defaultRenderer === 'dagre-wrapper') {
return false;
}
if (config?.flowchart?.defaultRenderer === 'elk') {
if (
config?.flowchart?.defaultRenderer === 'dagre-wrapper' ||
config?.flowchart?.defaultRenderer === 'elk'
) {
return false;
}
return txt.match(/^\s*graph/) !== null;
Expand Down
2 changes: 2 additions & 0 deletions packages/mermaid/src/mermaid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { ParseErrorFunction } from './Diagram';
import { isDetailedError } from './utils';
import type { DetailedError } from './utils';
import { ExternalDiagramDefinition } from './diagram-api/types';
import { UnknownDiagramError } from './errors';

export type {
MermaidConfig,
Expand All @@ -20,6 +21,7 @@ export type {
ParseErrorFunction,
RenderResult,
ParseOptions,
UnknownDiagramError,
};

export interface RunOptions {
Expand Down
4 changes: 2 additions & 2 deletions packages/mermaid/src/mermaidAPI.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,7 @@ describe('mermaidAPI', () => {
).rejects.toThrow(
'Diagrams beginning with --- are not valid. ' +
'If you were trying to use a YAML front-matter, please ensure that ' +
"you've correctly opened and closed the YAML front-matter with unindented `---` blocks"
"you've correctly opened and closed the YAML front-matter with un-indented `---` blocks"
);
});
it('does not throw for a valid definition', async () => {
Expand All @@ -678,7 +678,7 @@ describe('mermaidAPI', () => {
await expect(
mermaidAPI.parse('this is not a mermaid diagram definition')
).rejects.toThrowErrorMatchingInlineSnapshot(
'"No diagram type detected for text: this is not a mermaid diagram definition"'
'"No diagram type detected matching given configuration for text: this is not a mermaid diagram definition"'
);
});
it('returns false for invalid definition with silent option', async () => {
Expand Down