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: move from objects to Maps #5468

Merged
merged 20 commits into from
May 16, 2024
Merged

fix: move from objects to Maps #5468

merged 20 commits into from
May 16, 2024

Conversation

Yash-Singh1
Copy link
Member

@Yash-Singh1 Yash-Singh1 commented Apr 17, 2024

📑 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

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

netlify bot commented Apr 17, 2024

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...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

codecov bot commented Apr 17, 2024

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

Impacted file tree graph

@@            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

@Yash-Singh1 Yash-Singh1 requested a review from sidharthv96 April 23, 2024 15:40
Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

@sidharthv96
Copy link
Member

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

@Yash-Singh1
Copy link
Member Author

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

@sidharthv96
Copy link
Member

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

Copy link
Member

@Yokozuna59 Yokozuna59 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
packages/mermaid/src/diagrams/class/classDb.ts Outdated Show resolved Hide resolved
@sidharthv96
Copy link
Member

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

@sidharthv96 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 sidharthv96 deleted the fix/maps branch May 16, 2024 07:42
Copy link

mermaid-bot bot commented May 16, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Using constructor as node ID results in errors
4 participants