fix: move from objects to Maps #5468

Yash-Singh1 · 2024-04-17T16:01:25Z

📑 Summary

Switches from objects to Maps in places where the keys accepted are user inputs.

Resolves #5451

📏 Design Decisions

Describe the way your implementation works or what design decisions you made if applicable.

This is a breaking change for v11 since it changes the structure of the databases and as such can break code that relies on the db, e.g.

https://github.com/excalidraw/mermaid-to-excalidraw/blob/d0b00dc63eeabd5e92ca3997650f47bf4901cb2c/src/parser/flowchart.ts#L226

Why?

The main reason for this change was so that we wouldn't have to validate for constructor and __proto__ (#5451) everytime we have to modify or get a property on the database objects. Another good reason would be that objects in JavaScript are generally designed to work like dynamic structs, where the JIT compiler would store the shape of the object and rereference that shape everytime that object is passed around. However, when we have a "map" with user-generated keys, Maps work better because they are designed for that purpose.

📋 Tasks

Make sure you

📖 have read the contribution guidelines
💻 have added necessary unit/e2e tests.
📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
🔖 targeted develop branch

netlify · 2024-04-17T16:01:40Z

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	`00eaebe`
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/6644119193947100088ea8b7
😎 Deploy Preview	https://deploy-preview-5468--mermaid-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-04-17T17:30:02Z

Codecov Report

Attention: Patch coverage is 0.50251% with 396 lines in your changes are missing coverage. Please review.

Project coverage is 5.73%. Comparing base (0326d89) to head (00eaebe).

Additional details and impacted files

@@            Coverage Diff             @@
##           develop   #5468      +/-   ##
==========================================
- Coverage     5.74%   5.73%   -0.01%     
==========================================
  Files          277     278       +1     
  Lines        41967   41991      +24     
  Branches       515     490      -25     
==========================================
  Hits          2409    2409              
- Misses       39558   39582      +24

Flag	Coverage Δ
unit	`5.73% <0.50%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
packages/mermaid/src/diagrams/gantt/ganttDb.js	`77.47% <66.66%> (ø)`
packages/mermaid/src/diagrams/sequence/svgDraw.js	`0.00% <0.00%> (ø)`
packages/mermaid/src/diagrams/class/classTypes.ts	`0.00% <0.00%> (ø)`
...ckages/mermaid/src/diagrams/gantt/ganttRenderer.js	`0.00% <0.00%> (ø)`
...ages/mermaid-flowchart-elk/src/flowRenderer-elk.js	`0.00% <0.00%> (ø)`
...ckages/mermaid/src/diagrams/class/classRenderer.js	`0.00% <0.00%> (ø)`
...ges/mermaid/src/diagrams/flowchart/flowRenderer.js	`0.00% <0.00%> (ø)`
packages/mermaid/src/diagrams/pie/pieRenderer.ts	`0.00% <0.00%> (ø)`
packages/mermaid/src/diagrams/er/erRenderer.js	`0.00% <0.00%> (ø)`
packages/mermaid/src/diagrams/pie/pieDb.ts	`0.00% <0.00%> (ø)`
... and 17 more

... and 1 file with indirect coverage changes

sidharthv96

Appreciate the effort! Can you take a look at the E2E failures?

sidharthv96 · 2024-05-01T17:35:02Z

@Yash-Singh1 were you able to fix the tests?
Keeping this PR open for long will create more conflicts for you.

Yash-Singh1 · 2024-05-04T22:39:13Z

Hi @sidharthv96, sorry for the late reply, I was busy with exams. I just fixed the e2e tests.

sidharthv96 · 2024-05-05T14:46:30Z

No worries @Yash-Singh1! Hope they went well :)
Let's wait for a review from someone else as well, as the size is big.

Yokozuna59

Thanks for the great work!

I have a few comments. Is there a reason for some tests to be different? Like, some of them use it.each and some don't?

And I just would like the PR description to have the "why". Like listing the possible limitations of objects or downsides.

packages/mermaid/src/diagrams/flowchart/flowDb.spec.ts

packages/mermaid/src/diagrams/sequence/sequenceDiagram.spec.js

Remove the non-null assertion operator when possible from PR #5468

aloisklink

Awesome work @Yash-Singh1!!!

This is something I've wanted to do for a while too, so it's awesome to see you've done it (I can imagine that all the .js files must have been really painful without TypeScript).

I've got some minor nitpicks about not using the ! non-null assertion in TypeScript, but I've made a separate PR: #5518 for fixing this. You're welcome to merge it into your branch, or maybe we can merge this PR first, and then the other refactoring PR can also land.

packages/mermaid/src/diagrams/block/blockDB.ts

packages/mermaid/src/diagrams/class/classDb.ts

sidharthv96 · 2024-05-14T16:46:15Z

Merging this now, as both the review comments are non blockers, and can be done in a follow up PR.

…ipt-non-null-assertion-when-possible

mermaid-bot · 2024-05-16T07:42:19Z

@Yash-Singh1, Thank you for the contribution!
You are now eligible for a year of Premium account on MermaidChart.
Sign up with your GitHub account to activate.

Yash-Singh1 and others added 7 commits April 16, 2024 13:52

fix: move from objects to Maps

07de090

test: unsafe props to make sure they work with changes

002aa29

test: block, er, gantt, git, pie

c993a81

test: requirement, sankey

6105185

test: seq, state

087a574

test: sankey, pie, seq diagram

9b411fa

Update docs

357da0c

github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Apr 17, 2024

Yash-Singh1 requested a review from sidharthv96 April 23, 2024 15:40

sidharthv96 requested changes Apr 24, 2024

View reviewed changes

fix: use maps for quadrant diagrams

128c69a

Yash-Singh1 force-pushed the fix/maps branch from f730612 to 128c69a Compare April 25, 2024 02:15

fix: some changes

92f1941

Yash-Singh1 force-pushed the fix/maps branch from 53bb5eb to 92f1941 Compare May 2, 2024 22:29

Yash-Singh1 added 7 commits May 2, 2024 15:34

Merge branch 'develop' into fix/maps

e68ba3f

fix: git graph diffs

6f1a553

fix: class diagram notes plss

1cbd730

format

e2ca8e8

fix: classrenderer v2 using in op

58af763

trigger e2e again

ca4fa11

chore: rm autocompleted import

50c9ede

sidharthv96 approved these changes May 5, 2024

View reviewed changes

sidharthv96 requested review from knsv, ashishjain0512, aloisklink and Yokozuna59 May 5, 2024 14:45

Yokozuna59 reviewed May 10, 2024

View reviewed changes

packages/mermaid/src/diagrams/flowchart/flowDb.spec.ts Outdated Show resolved Hide resolved

packages/mermaid/src/diagrams/sequence/sequenceDiagram.spec.js Outdated Show resolved Hide resolved

refactor: remove non-null assertion operator

730fa89

Remove the non-null assertion operator when possible from PR #5468

aloisklink mentioned this pull request May 13, 2024

[PR#5468] refactor: remove TypeScript non null assertions #5518

Merged

4 tasks

aloisklink approved these changes May 13, 2024

View reviewed changes

packages/mermaid/src/diagrams/block/blockDB.ts Outdated Show resolved Hide resolved

packages/mermaid/src/diagrams/class/classDb.ts Outdated Show resolved Hide resolved

Merge branch 'develop' into fix/maps

2df26ca

Yash-Singh1 added 2 commits May 14, 2024 18:33

chore: address @Yokozuna59's code reviews

dd92aec

Merge pull request #5518 from aloisklink/5468/refactor/remove-TypeScr…

00eaebe

…ipt-non-null-assertion-when-possible

sidharthv96 added this pull request to the merge queue May 16, 2024

Merged via the queue into develop with commit 8a5fe53 May 16, 2024
29 checks passed

sidharthv96 deleted the fix/maps branch May 16, 2024 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: move from objects to Maps #5468

fix: move from objects to Maps #5468

Yash-Singh1 commented Apr 17, 2024 •

edited

netlify bot commented Apr 17, 2024 •

edited

codecov bot commented Apr 17, 2024 •

edited

sidharthv96 left a comment

sidharthv96 commented May 1, 2024

Yash-Singh1 commented May 4, 2024

sidharthv96 commented May 5, 2024

Yokozuna59 left a comment

aloisklink left a comment

sidharthv96 commented May 14, 2024

mermaid-bot bot commented May 16, 2024

fix: move from objects to Maps #5468

fix: move from objects to Maps #5468

Conversation

Yash-Singh1 commented Apr 17, 2024 • edited

📑 Summary

📏 Design Decisions

Why?

📋 Tasks

netlify bot commented Apr 17, 2024 • edited

✅ Deploy Preview for mermaid-js ready!

codecov bot commented Apr 17, 2024 • edited

Codecov Report

sidharthv96 left a comment

Choose a reason for hiding this comment

sidharthv96 commented May 1, 2024

Yash-Singh1 commented May 4, 2024

sidharthv96 commented May 5, 2024

Yokozuna59 left a comment

Choose a reason for hiding this comment

aloisklink left a comment

Choose a reason for hiding this comment

sidharthv96 commented May 14, 2024

mermaid-bot bot commented May 16, 2024

Yash-Singh1 commented Apr 17, 2024 •

edited

netlify bot commented Apr 17, 2024 •

edited

codecov bot commented Apr 17, 2024 •

edited