fix/1294_exhaustive-clear-sequenceDb-variables #4941

rflban · 2023-10-12T13:51:59Z

📑 Summary

Reason of bugs like one in issue are global variables cached between renders. I've made exhaustive cleanup of sequenceDb.

You can reproduce the bug in mermaid live editor with these diagrams:

sequenceDiagram
    Alice->>Bob: Hello Bob, how are you ?
    Bob->>Alice: Fine, thank you. And you?
    create participant Carl
    Alice->>Carl: Hi Carl!
    create actor D as Donald
    Carl->>D: Hi!
    destroy Carl
    Alice-xCarl: We are too many
    destroy Bo
    Bob->>Alice: I agree

sequenceDiagram
    Alice->>Bob: Hello Bob, how are you ?
    Bob->>Alice: Fine, thank you. And you?
    create participant Carl
    Alice->>Carl: Hi Carl!
    create actor D as Donald
    Carl->>D: Hi!
    destroy Carl
    Alice-xCarl: We are too many
    destroy Bob
    Bob->>Alice: I agree

Resolves #1294

📏 Design Decisions

I've added imperative state. It's simple structure, that is created from init function and provides reset method, that recalls init function to restore initial values of state's fields. This approach ensures that newly added state fields are cleared.

Also I've added two identity functions, that help specify type of state initial values for typescript.

📋 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 · 2023-10-12T13:52:05Z

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	`1b20fe9`
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/659f6b421d64d70008b24152
😎 Deploy Preview	https://deploy-preview-4941--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 · 2023-10-12T13:58:51Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (34e0942) 43.04% compared to head (1b20fe9) 79.70%.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #4941       +/-   ##
============================================
+ Coverage    43.04%   79.70%   +36.65%     
============================================
  Files           23      167      +144     
  Lines         5018    13873     +8855     
  Branches        21      707      +686     
============================================
+ Hits          2160    11057     +8897     
+ Misses        2857     2663      -194     
- Partials         1      153      +152

Flag	Coverage Δ
e2e	`85.60% <96.22%> (?)`
unit	`43.04% <ø> (ø)`

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

Files	Coverage Δ
packages/mermaid/src/utils/imperativeState.ts	`100.00% <100.00%> (ø)`
...ckages/mermaid/src/diagrams/sequence/sequenceDb.js	`84.03% <96.00%> (ø)`

... and 161 files with indirect coverage changes

…equenceDb-variables

sidharthv96

This is splendid, and long overdue!
@Yokozuna59 has been doing related work in other diagrams, so their review would be good to have.

Few minor issues only, great job!

packages/mermaid/src/docs/syntax/sequenceDiagram.md

packages/mermaid/src/utils/imperativeState.ts

cypress/helpers/util.ts

…ram.md Co-authored-by: Sidharth Vinod <[email protected]>

…ables

aloisklink

This looks awesome :)

I love how you've managed to change the code in a way that should prevent a bug like #1294 from happening again too!

The only thing I don't 100% love is the syntax, which is a bit unusual to me. I feel like using something like the following would more closely match the rest of the JavaScript eco-system:

state = new ImperativeState(() => {
 // default state
});

I've already even got a commit for it: aloisklink@35191a6 that I could push to this branch.

packages/mermaid/src/utils/imperativeState.ts

sidharthv96 · 2023-11-25T04:57:11Z

@rflban can you integrate the suggestions and update the PR? Would love to merge this soon.

nirname

I love this PR. My general thoughts about DB is that instead of "cleaning" function we have to recreate DB as an object, so our overall flow would be

create db
populate it with data from parser
render it

Every call of "clean" basically equals object destruction. So I love the overall approach and I think it have to be pushed further than that.

I agree on the comments from other reviewers, it's better to address them. Lets wait for those minor fixes, and this is good to go. Just comment from me this time, not approve, neither reject.

sidharthv96 · 2023-12-04T04:21:41Z

@rflban Do you have time to address the review comments, or can we push the changes and merge it? Would like to include this in the next release.

rflban · 2023-12-04T08:57:20Z

@rflban Do you have time to address the review comments, or can we push the changes and merge it? Would like to include this in the next release.

Sorry, I haven't notice the new comments. I will fix the comments today

sidharthv96 · 2023-12-07T05:26:20Z

@rflban sorry to bother you. This is an excellent candidate for what might be the last v10 release (which we're hoping to do by next week). Otherwise we would've moved it out of the queue.

rflban · 2023-12-07T08:39:17Z

@rflban sorry to bother you. This is an excellent candidate for what might be the last v10 release (which we're hoping to do by next week). Otherwise we would've moved it out of the queue.

It's my fault, I did not have time to commit changes. But this pr is ready to check now

aloisklink

Looks good to me! Thanks for making those changes!

@sidharthv96, do you want to double-check that all of your "Requested changes" have been dealt with before merging this PR? GitHub is blocking me from merging until all conversations have been resolved.

packages/mermaid/src/utils/imperativeState.ts

* develop: (200 commits) chore(deps): update all minor dependencies adds corresponding change in docs/ecosystem Adds Unison programming language to community integrations list Fixed parser/tests Update docs Update classDiagram.md Update classDiagram.md Update docs Update packages/mermaid/src/diagrams/class/classDb.ts Update packages/mermaid/src/docs/syntax/classDiagram.md Update packages/mermaid/src/diagrams/class/classDb.ts chore(deps): update all minor dependencies Update generics docs Update docs Address potential undefined refactor: Move maxEdges out of flowchart config. refactor: Move maxEdges out of flowchart config. chore: Add maxEdges to secure list Update docs Update NiceGuy.io links in integrations-community.md ...

mermaid-bot · 2024-01-11T04:52:21Z

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

fix: add imperativeState and replace sequenceDb global variables with it

b4f4448

github-actions bot added the fix label Oct 12, 2023

rflban changed the title ~~fix/1294_exhaustive-clear-sequenceDb-variables~~ Draft: fix/1294_exhaustive-clear-sequenceDb-variables Oct 12, 2023

rflban changed the title ~~Draft: fix/1294_exhaustive-clear-sequenceDb-variables~~ fix/1294_exhaustive-clear-sequenceDb-variables Oct 12, 2023

rflban added 4 commits October 13, 2023 00:02

chore: add e2e test that shows db cleanup problem

18ea27a

Merge branch 'fix/1294_add-e2e-test' into fix/1294_exhaustive-clear-s…

4555245

…equenceDb-variables

chore: add e2e test that shows db cleanup problem

985eda2

chore: remove unused e2e tests file

6eae46b

rflban marked this pull request as draft October 12, 2023 21:50

chore(sequence): update doc for actors/participant creation/deletion fix

fc0ade2

rflban marked this pull request as ready for review October 13, 2023 10:41

sidharthv96 requested changes Oct 16, 2023

View reviewed changes

rflban and others added 2 commits October 16, 2023 19:36

chore(sequence): Update packages/mermaid/src/docs/syntax/sequenceDiag…

3128ba7

…ram.md Co-authored-by: Sidharth Vinod <[email protected]>

chore: fix autogen docs

3b5f5c7

rflban requested a review from sidharthv96 October 18, 2023 14:48

Merge branch 'develop' into fix/1294_exhaustive-clear-sequenceDb-vari…

61fb9e5

…ables

sidharthv96 requested review from nirname, aloisklink and Yokozuna59 November 9, 2023 18:05

aloisklink reviewed Nov 14, 2023

View reviewed changes

packages/mermaid/src/utils/imperativeState.ts Outdated Show resolved Hide resolved

packages/mermaid/src/utils/imperativeState.ts Outdated Show resolved Hide resolved

aloisklink added the Graph: Sequence label Nov 14, 2023

nirname reviewed Nov 26, 2023

View reviewed changes

sidharthv96 added this to the v10 milestone Nov 28, 2023

fix: replace functional approaches with oop

b60fc1b

github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Dec 7, 2023

rflban requested review from nirname and aloisklink December 7, 2023 08:37

aloisklink approved these changes Dec 7, 2023

View reviewed changes

sidharthv96 reviewed Dec 10, 2023

View reviewed changes

packages/mermaid/src/utils/imperativeState.ts Outdated Show resolved Hide resolved

packages/mermaid/src/utils/imperativeState.ts Outdated Show resolved Hide resolved

sidharthv96 added 2 commits January 11, 2024 09:42

chore: Minor cleanup of imperativeState

0ac3394

sidharthv96 added this pull request to the merge queue Jan 11, 2024

Merged via the queue into mermaid-js:develop with commit 54446f1 Jan 11, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/1294_exhaustive-clear-sequenceDb-variables #4941

fix/1294_exhaustive-clear-sequenceDb-variables #4941

rflban commented Oct 12, 2023 •

edited

Loading

netlify bot commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading

sidharthv96 left a comment

aloisklink left a comment

sidharthv96 commented Nov 25, 2023

nirname left a comment

sidharthv96 commented Dec 4, 2023

rflban commented Dec 4, 2023

sidharthv96 commented Dec 7, 2023

rflban commented Dec 7, 2023

aloisklink left a comment

mermaid-bot bot commented Jan 11, 2024

fix/1294_exhaustive-clear-sequenceDb-variables #4941

fix/1294_exhaustive-clear-sequenceDb-variables #4941

Conversation

rflban commented Oct 12, 2023 • edited Loading

📑 Summary

📏 Design Decisions

📋 Tasks

netlify bot commented Oct 12, 2023 • edited Loading

✅ Deploy Preview for mermaid-js ready!

codecov bot commented Oct 12, 2023 • edited Loading

Codecov Report

sidharthv96 left a comment

Choose a reason for hiding this comment

aloisklink left a comment

Choose a reason for hiding this comment

sidharthv96 commented Nov 25, 2023

nirname left a comment

Choose a reason for hiding this comment

sidharthv96 commented Dec 4, 2023

rflban commented Dec 4, 2023

sidharthv96 commented Dec 7, 2023

rflban commented Dec 7, 2023

aloisklink left a comment

Choose a reason for hiding this comment

mermaid-bot bot commented Jan 11, 2024

rflban commented Oct 12, 2023 •

edited

Loading

netlify bot commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading