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

Refreshing page will show Syntax error in mermaid graph sometimes #3232

Closed
5 tasks done
orenwang opened this issue Nov 17, 2021 · 19 comments
Closed
5 tasks done

Refreshing page will show Syntax error in mermaid graph sometimes #3232

orenwang opened this issue Nov 17, 2021 · 19 comments
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open

Comments

@orenwang
Copy link

orenwang commented Nov 17, 2021

Contribution guidelines

I've found a bug and checked that ...

  • ... the problem doesn't occur with the mkdocs or readthedocs themes
  • ... the problem persists when all overrides are removed, i.e. custom_dir, extra_javascript and extra_css
  • ... the documentation does not mention anything about my problem
  • ... there are no open or closed issues that are related to my problem

Description

Refreshing page will show Syntax error in mermaid graph sometimes. This shows in the Official Docs for me. Also, using mkdocs serve/build reproduces the same bug.

image

Expected behaviour

Always show the correctly rendered MermaidJS graph.

Actual behaviour

Sometimes normal, while sometimes not.

Refreshing page but with cache disabled can solve this problem...

Steps to reproduce

Just keep refreshing the page.

Package versions

Doesn't matter.

Configuration

Doesn't matter.

System information

  • Operating system: Mac OS / Windows.
  • Browser: Chrome / Firefox
@squidfunk
Copy link
Owner

Thanks for reporting! It's not reproducible for me in Chrome but in Firefox.

@squidfunk squidfunk added the bug Issue reports a bug label Nov 17, 2021
@squidfunk
Copy link
Owner

squidfunk commented Nov 17, 2021

The semantics of startOnLoad seem to have changed with the latest update. Likely related:

@orenwang
Copy link
Author

orenwang commented Nov 17, 2021

The semantics of startOnLoad seem to have changed with the latest update. Likely related:

Thanks for the quick response @squidfunk . I've tested earlier with prior versions to the aforementioned MR of mkdocs-material locally, which brings older version of MermaidJS too. Same problem. Still no clue what is the root cause.
Also, totally fine on websites with MermaidJS alone (without mkdocs-material). Thus, I suspect maybe the <pre> or <code> tags bring some confusions to MermaidJS.

@squidfunk
Copy link
Owner

squidfunk commented Nov 17, 2021

The problem is that the moment Mermaid.js is loaded it will immediately try and parse all elements with the .mermaid class. The documentation says that startOnLoad should be set to false which we do here, but this does not work properly. If Mermaid.js is loaded from cache, Mermaid will sometimes not pick up the configuration and start processing before one can tell it to not startOnLoad.

@squidfunk
Copy link
Owner

squidfunk commented Nov 17, 2021

Potential fix in f8b74c207. My testing shows that this mitigates the issue. I'll look into it again to search for a better solution when I find some time to refactor this part of the code. It's really sad that Mermaid has such a bad API design.

@squidfunk squidfunk added the resolved Issue is resolved, yet unreleased if open label Nov 17, 2021
@facelessuser
Copy link
Contributor

I've actually never seen this issue. I am not currently using Material's solution yet, but I've never had to strip off the class and add it back. I never see the syntax error. I've tried refreshing a bunch in my own docs...

@facelessuser
Copy link
Contributor

I guess I can't reproduce it in the official docs either... 🤷🏻

@facelessuser
Copy link
Contributor

I take it back. I can reproduce it in Firefox, but only with Material's solution. So I must be doing something different in my solution than what Material is doing. I do not have to strip out mermaid classes and then add them back.

@squidfunk
Copy link
Owner

squidfunk commented Nov 17, 2021

Yes, it's related to the usage of pre/code blocks which Mermaid chokes on, but which are a better choice than div because they can be reliably run through the minifier. IMHO, temporarily removing the class is the only option, as it will leave the auto-mounting logic with nothing to render.

@facelessuser
Copy link
Contributor

That's weird as I am also using pre/code but having no issues. It must be something with how Material is specifically doing things.

@squidfunk
Copy link
Owner

How are you initializing/loading Mermaid?

@facelessuser
Copy link
Contributor

I don't think Mermaid runs async, but I do it the same way as I do MathJax which does use async 🤷🏻 :

I don't know how much any of this info will help, but maybe there is something that stands out.

@orenwang
Copy link
Author

orenwang commented Nov 18, 2021

Thanks for the fix. It works perfectly for me now!

Removing & adding back the class selector does seem a bit hacky but shouldn't be a problem as I see it.

@squidfunk
Copy link
Owner

squidfunk commented Nov 18, 2021

@facelessuser I guess the problem lies within the fact that Material will automatically include Mermaid.js when it sees an element tagged with a .mermaid class. The configuration you use is more predictable because Mermaid is always loaded in a deterministic way. Since Material will auto-mount, it's hardly possible to stop Mermaid when it's loaded from cache, since it will start running right away – no chance to tell it to startOnLoad: false. I've also tried this with the deprecated global settings to no avail. For this reason, I believe removing/adding back the CSS class is the most solid approach, since you're taking everything from Mermaid away it could automatically run on, and run it explicitly afterward.

A better approach would be to patch Mermaid to not run automatically, but since it is so widely used and it would be a breaking change, I'd suspect that it would be too hard to convince the author to do so.

@facelessuser
Copy link
Contributor

Ah, makes sense.

@squidfunk
Copy link
Owner

The fix was just released as part of 7.3.6-insiders-3.2.3.

@Guts
Copy link
Contributor

Guts commented Nov 26, 2021

Thanks a lot @orenwang, this is exactly the problem that has been blocking us for a while: #3056. Hurray, it's finally fixed 🙏!

@orenwang
Copy link
Author

@Guts You are welcome. Glad it helped!

@zacharysarah
Copy link

Thanks for this discussion--it helped us diagnose a similar issue on the Kubernetes website. 🙇🏻

Hootsdev pushed a commit to HOOTORO/afk.GG that referenced this issue Jul 5, 2023
squidfunk/mkdocs-material#3232
this issue was guidance to root case.
Turn off nav.instant and js load order comeback to normal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open
Projects
None yet
Development

No branches or pull requests

5 participants