fix: prevent duplicate ID generation in fast multi-block renders#6621
fix: prevent duplicate ID generation in fast multi-block renders#6621binary8421 wants to merge 3 commits intomermaid-js:developfrom
Conversation
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mermaid
@mermaid-js/layout-elk
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6621 +/- ##
==========================================
+ Coverage 3.87% 3.89% +0.01%
==========================================
Files 414 413 -1
Lines 43303 43334 +31
Branches 666 666
==========================================
+ Hits 1679 1687 +8
- Misses 41624 41647 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
| // v11: Use the actual value of seed string to generate an initial value for count. | ||
| this.count = seed ? seed.length : 0; | ||
| this.next = deterministic ? () => this.count++ : () => Date.now(); | ||
| this.next = deterministic ? () => this.count++ : () => TimeProvider.getTimestamp(); |
There was a problem hiding this comment.
Doing this should solve the problem of duplicate ID, and we won't have to add the complexity of TimeProvider.
We don't have a requirement that the ID should be the timestamp.
| this.next = deterministic ? () => this.count++ : () => TimeProvider.getTimestamp(); | |
| this.next = deterministic | |
| ? () => this.count++ | |
| : () => Number.parseInt(`${Date.now()}${this.count++}`); |
Or even simpler, we could concatenate it and return the string, we'd have to fix the tests to match the functionality though.
There was a problem hiding this comment.
Thank you for your input. From the perspective of long-term project maintenance, I’d like to explain why I believe TimeProvider is a suitable choice:
Timestamp Precision and Uniqueness:
By leveraging performance.now() for higher precision and ensuring monotonic increments, TimeProvider guarantees the generation of unique IDs.
Cross-Platform Compatibility:
TimeProvider is designed to work reliably across diverse environments, such as Node.js or older browsers, by handling differences in timing APIs.
Maintainability and Extensibility:
The modular design of TimeProvider encapsulates timestamp generation logic, making it easier to maintain and extend. If future requirements demand adjustments to timestamp precision or uniqueness strategies, changes can be made within TimeProvider without impacting the calling code.
Test Friendliness:
By building on the existing project structure, TimeProvider minimizes conceptual changes, allowing the current test cases to remain intact without requiring modifications.
I hope this clarifies the rationale behind using TimeProvider. If there are specific concerns or alternative approaches you’d like to explore, I’m happy to discuss further to find the best solution for the project!
There was a problem hiding this comment.
Are you using AI for this? 🤔
By the way, since this class is created every time, the "+1" operation has no effect. Co-authored-by: Sidharth Vinod <github@sidharth.dev>
binary8421
left a comment
There was a problem hiding this comment.
I've tested your code, and ID conflicts still occur. Detailed comments have been added.
| this.next = deterministic ? () => this.count++ : () => TimeProvider.getTimestamp(); | ||
| this.next = deterministic | ||
| ? () => this.count++ | ||
| : () => Number.parseInt(`${Date.now()}${this.count++}`); |
There was a problem hiding this comment.
By the way, since this class is created every time, the "+1" operation has no effect.
There was a problem hiding this comment.
I think the implementation has diverged somehow. My expectation was the IdGenerator was for individual nodes of the diagram, as the doc suggests. But currently it's only used for the ID of the diagram (which makes the deterministic flag pointless, as you said).
mermaid/packages/mermaid/src/config.type.ts
Lines 164 to 174 in 797ba43
There was a problem hiding this comment.
In this case, we'd be better off just using a cuid for diagram id. Which has timestamp and a random component.
| // TODO: Seed is only used for length? | ||
| // v11: Use the actual value of seed string to generate an initial value for count. | ||
| this.count = seed ? seed.length : 0; | ||
| this.next = deterministic ? () => this.count++ : () => TimeProvider.getTimestamp(); |
There was a problem hiding this comment.
Using a constant object to handle conflicts is a good approach.
|
Are there any follow-up plans?Rendering multiple nodes simultaneously does indeed lead to duplicate ids |
|
@binary8421 the IDGenerator is created once per invocation of @EnochGao This PR only handles IDs for diagrams, not nodes. Can you clarify your use case as well? |
📑 Summary
Fix duplicate ID generation when rendering multiple Mermaid blocks quickly.
📏 Design Decisions
This fix changes the ID generation to use microsecond-level timestamps instead of milliseconds, greatly reducing the chance of duplicate IDs.
Additionally, if a duplicate ID is detected, an incremental counter is added to ensure uniqueness.
📋 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:.