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: call .node to check for node existence #5448

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

FezVrasta
Copy link

📑 Summary

Brief description about the content of your PR.

Resolves #

📏 Design Decisions

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

📋 Tasks

Make sure you

Copy link

netlify bot commented Apr 8, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit ff34dbc
🔍 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
Copy link
Author

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
Copy link
Member

@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
Copy link
Author

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

@sidharthv96
Copy link
Member

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

@FezVrasta
Copy link
Author

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
Copy link
Member

sidharthv96 commented Apr 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants