-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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: getMessageAPI so it considers entity codes #5002
fix: getMessageAPI so it considers entity codes #5002
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #5002 +/- ##
===========================================
- Coverage 79.37% 79.35% -0.02%
===========================================
Files 164 164
Lines 13820 13842 +22
Branches 693 693
===========================================
+ Hits 10969 10984 +15
- Misses 2702 2709 +7
Partials 149 149
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@sidharthv96 can you take a look at the PR and share your insights 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see tests!
If moving to constructor and splitting the functions to a different file doesn't cause issues, it should be good to go.
Also, curious to know the usecase for directly accessing the inner functions.
These could break in a while as we switch to a new internal architecture.
In Excalidraw we are adding support for mermaid hence using these internal function to get the diagram data from text. |
txt = txt.replace(/style.*:\S*#.*;/g, function (s): string { | ||
return s.substring(0, s.length - 1); | ||
}); |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
regular expression
library input
This
regular expression
library input
This
regular expression
library input
@sidharthv96 I have made the changes, however I am not sure what docs I am suppose to update (its throwing lint error), could you guide me here ? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the encode entities function invocation into the diagram constructor.
@ad1992 you can run |
@sidharthv96 made the changes, the PR should be good to go! |
Thanks @ad1992! |
@ad1992, Thank you for the contribution! |
fixes #4983
📑 Summary
This issue is reproducible when using the API
diagram.parser.yy.getMessages
ordiagram.db.getMessages
API directly eg in test since the entity codes are not handled ingetDiagramFromText
API.In mermaid it was working fine since the entity codes were being handled before calling the API
getDiagramFromText
hence I have removed that and instead handling it inside the functiongetDiagramFromText
.I hope this should be acceptable and I have added test as well for the same.
Resolves #4983
📏 Design Decisions
Describe the way your implementation works or what design decisions you made if applicable.
Already explained above
📋 Tasks
Already explained above
As per the lint rule failing for circular deps, I think we can move the util
encodeEntities
topackages/mermaid/src/utils.ts
since now its a reusable common util. let me know If I should move the util.Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.develop
branch