Refactor: Change flowDB to class based architecture. #6161
Conversation
✅ 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 #6161 +/- ##
=======================================
Coverage 4.47% 4.48%
=======================================
Files 383 383
Lines 54144 54124 -20
Branches 622 622
=======================================
Hits 2425 2425
+ Misses 51719 51699 -20
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎ Awaiting the start of a new Argos build… |
🦋 Changeset detectedLatest commit: f7fe8f2 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 |
There was a problem hiding this comment.
Looks great! Thank you for making this PR as easy to review as possible!
My biggest issues that I've spotted are:
- We're using class fields and arrow functions instead of class methods, which is a bit strange and not as well supported on browsers. Why can't we just use normal class methods?
- I think we need a changeset entry. Maybe we can make a
fix: XXXXXorpatchchangeset release? Saying that thedbreturned frommermaid.mermaidAPI.getDiagramFromText()is now unique for flowcharts? From doing a search, it looks like ~27 people are usingmermaidAPI.getDiagramFromText()already: https://github.com/search?q=mermaidAPI.getDiagramFromText&type=code
Co-authored-by: Alois Klink <[email protected]>
Co-authored-by: Alois Klink <[email protected]>
ashishjain0512
changed the title
…saurabh/refactor/convert-flowDb-to-class
| this.vertexCounter++; | ||
|
|
||
| if (textObj !== undefined) { | ||
| this.config = getConfig(); |
There was a problem hiding this comment.
Why do we need to update the config when adding every vertex?
I see this is from the old code, so you can leave it in now, and fix later.
| private config = getConfig(); | ||
| private vertices = new Map<string, FlowVertex>(); | ||
| private edges: FlowEdge[] & { defaultInterpolate?: string; defaultStyle?: string[] } = []; | ||
| private classes = new Map<string, FlowClass>(); | ||
| private subGraphs: FlowSubGraph[] = []; | ||
| private subGraphLookup = new Map<string, FlowSubGraph>(); | ||
| private tooltips = new Map<string, string>(); | ||
| private subCount = 0; | ||
| private firstGraphFlag = true; | ||
| private direction: string | undefined; | ||
|
|
||
| private version: string | undefined; // As in graph | ||
|
|
||
| private secCount = -1; | ||
| private posCrossRef: number[] = []; | ||
|
|
||
| // Functions to be run after graph rendering | ||
| private funs: ((element: Element) => void)[] = []; // cspell:ignore funs | ||
|
|
||
| constructor() { | ||
| this.funs.push(this.setupToolTips); | ||
| this.clear(); |
There was a problem hiding this comment.
getConfig is called when initailizing, and also inside this.clear function, so one call is redundant.
There was a problem hiding this comment.
I think that in order to keep this PR's change surface as small as possible, we could leave this in and fix this in another PR.
With this new class style, we might not even need a .clear() function (except maybe to clear the common DB values?).
| public defaultConfig = () => defaultConfig.flowchart; | ||
| public setAccTitle = setAccTitle; | ||
| public setAccDescription = setAccDescription; | ||
| public setDiagramTitle = setDiagramTitle; | ||
| public getAccTitle = getAccTitle; | ||
| public getAccDescription = getAccDescription; | ||
| public getDiagramTitle = getDiagramTitle; |
There was a problem hiding this comment.
Hhhhmmmmmmmmmmmm, this means part of the DB will be tied to a diagram, and some parts will change based on the last rendered diagram.
There was a problem hiding this comment.
Yep, it's probably something to fix next.
I'd imagine it would be more difficult, though, since I believe the Langium parsers also modify this shared state.
And since it's part of the DiagramAPI in
mermaid/packages/mermaid/src/diagram-api/types.ts
Lines 43 to 57 in 037ba2f
@saurabhg772244, I know you've already documented in the PR description that you haven't changed these functions. Can you maybe add a link to this comment thread to explain why we haven't changed them in this PR?
| this.funs.push(this.setupToolTips); | ||
| this.clear(); | ||
| this.setGen('gen-2'); |
There was a problem hiding this comment.
This is already done inside clear, so why do we need to do it here?
There was a problem hiding this comment.
They're copied from the old init call in:
mermaid/packages/mermaid/src/diagrams/flowchart/flowDiagram.ts
Lines 23 to 24 in bc2cc61
But @saurabhg772244 has changed .clear() to use 'gen-2' by default, see #6161 (comment)
IMO, my gut feeling is that it's probably better to change as little as possible in this PR, and maybe do the gen-1/gen-2 cleanup in a separate PR, to keep this PR as small as possible.
There was a problem hiding this comment.
We require gen-2 as gen-1 is deprecated and will break sub-graphs. https://mermaidchart.slack.com/archives/C06FGFCQVSR/p1735895537015209
There was a problem hiding this comment.
I think all your code looks good 1, but I'm still not sure if using arrow functions assigned to public instance fields is safe, since it looks like Safari 15.6 still had issues with it, which is a relatively recent browser.
Before merging this, can we make sure that this won't be a breaking change? Otherwise, we'd either want to make a Mermaid v12 release, or we should rewrite this PR so it still works on older browsers.
Footnotes
-
Maybe other than the
gen-1togen-2change in https://github.com/mermaid-js/mermaid/pull/6161#discussion_r1903932273, but that's really minor. ↩
| public defaultConfig = () => defaultConfig.flowchart; | ||
| public setAccTitle = setAccTitle; | ||
| public setAccDescription = setAccDescription; | ||
| public setDiagramTitle = setDiagramTitle; | ||
| public getAccTitle = getAccTitle; | ||
| public getAccDescription = getAccDescription; | ||
| public getDiagramTitle = getDiagramTitle; |
There was a problem hiding this comment.
Yep, it's probably something to fix next.
I'd imagine it would be more difficult, though, since I believe the Langium parsers also modify this shared state.
And since it's part of the DiagramAPI in
mermaid/packages/mermaid/src/diagram-api/types.ts
Lines 43 to 57 in 037ba2f
@saurabhg772244, I know you've already documented in the PR description that you haven't changed these functions. Can you maybe add a link to this comment thread to explain why we haven't changed them in this PR?
| this.funs.push(this.setupToolTips); | ||
| this.clear(); | ||
| this.setGen('gen-2'); |
There was a problem hiding this comment.
They're copied from the old init call in:
mermaid/packages/mermaid/src/diagrams/flowchart/flowDiagram.ts
Lines 23 to 24 in bc2cc61
But @saurabhg772244 has changed .clear() to use 'gen-2' by default, see #6161 (comment)
IMO, my gut feeling is that it's probably better to change as little as possible in this PR, and maybe do the gen-1/gen-2 cleanup in a separate PR, to keep this PR as small as possible.
…saurabh/refactor/convert-flowDb-to-class
📑 Summary
diagramObject.dbis called.📏 Design Decisions
flow.jison📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.