search

mermaid-js / mermaid

Generation of diagrams like flowcharts or sequence diagrams from text in a similar manner as markdown
https://mermaid.js.org
MIT License
68.75k stars 6.08k forks source link

fix: call .node to check for node existence #5448

Open FezVrasta opened 2 months ago

FezVrasta commented 2 months ago

:bookmark_tabs: Summary

Brief description about the content of your PR.

Resolves #

:straight_ruler: Design Decisions

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

:clipboard: Tasks

Make sure you

netlify[bot] commented 2 months ago

Deploy Preview for mermaid-js ready!

Name Link
Latest commit ff34dbc43a24652e464d2ef455fbb5cc17708f48
Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/6613a7384536560008741b45
Deploy Preview https://deploy-preview-5448--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.

FezVrasta commented 2 months ago

Unfortunately these changes are still not enough to avoid all the runtime errors related to missing DOM nodes. The fact .node() can return void means every time it's used there should be a check.

sidharthv96 commented 2 months ago

@FezVrasta can you share which issue this PR is trying to solve? If there is no existing issue, can you share what the errors that you are facing are?

FezVrasta commented 2 months ago

We are running mermaid on a React application, and we are encountering errors of the kind "cannot read getBBox from undefined".

sidharthv96 commented 2 months ago

Can you post an example mermaid code that has the issue?

FezVrasta commented 2 months ago

I'm unable to provide a repro at this time, but the PR fixes at least 1 type error (where you are forgetting to call node), I don't think a repro is required to fix type errors that can be verified by looking at the implementation of the method right?

sidharthv96 commented 2 months ago

I don't think a repro is required to fix type errors that can be verified by looking at the implementation of the method right?

No, it's not. The JS code does have code that sometimes feels like errors, but are infact guard clauses or workarounds to resolve other issues (it does have actual errors too :) ). There is an ongoing effort to migrate to TS, which has caught a lot of errors like this.

But in this case, the if condition was a guard clause, maybe to handle some unit test cases. So the proper fix would be to fix the test, and then remove the if (or call the .node()).

The following unit test failure is caused after calling the node.

image

And I requested the repro to find out what's happening at runtime.