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

Upgrade mermaid-js/mermaid @ 0918b97 (v8.8.0) #443

Merged
merged 3 commits into from
Sep 10, 2020

Conversation

mbbx6spp
Copy link
Contributor

@mbbx6spp mbbx6spp commented Sep 7, 2020

I upgraded the mermaid-js/mermaid static JS with the minimized version built by using the following method:

$ wget -q https://github.com/mermaid-js/mermaid/archive/8.8.0.zip
...
$ unzip mermaid-8.8.0.zip
$ cd mermaid-8.8.0/

$ nix-shell -p yarn --run "yarn install && yarn release" # equivalent to 'yarn install && yarn release' if yarn already installed
...

$ shasum -a 256 <./dist/mermaid.min.js
d27383a9754a526f5e46d6f39d0c17cdb66bbd2bf5285242e8abdf841831a2ec  -

The new mermaid has support for a lot more diagram types including:

  • stateDiagram-v2: state diagram
  • erDiagram: entity-relationship diagram
  • journey: user journey

More information here: https://mermaid-js.github.io/mermaid/diagrams-and-syntax-and-examples/n00b-syntaxReference.html

This PR also includes changes to the exampleSite that fix formatting in the mermaid shortcode examples for correct rendering and adds a new example for the stateDiagram-v2 for both English and French (please make suggestions to my French state machine for more appropriate words).

@mbbx6spp
Copy link
Contributor Author

mbbx6spp commented Sep 7, 2020

BTW I love this Learn theme! Thank you.

@mbbx6spp mbbx6spp changed the title Upgrade mermaid-js/mermaid @ 0918b97 Upgrade mermaid-js/mermaid @ 0918b97 (v8.8.0) Sep 7, 2020
@mbbx6spp
Copy link
Contributor Author

mbbx6spp commented Sep 7, 2020

This is explicitly not trying to solve the CDN issue mentioned in other issues. It is the dumbest upgrade path so that the Learn theme can support a newer mermaid sooner since all the other CDN resolving PRs did not get merged so far.

@matalo33
Copy link
Contributor

Thanks @mbbx6spp, let's get this merged while larger CDN discussion takes place. Also thanks for providing me steps to validate the contents of the javascript you provided, however I have mismatching sha256 on my side during validation.

Could I ask why you do a yarn build when the zip file already contains the minified js?

My steps to validate as follows...

$ curl -LO https://github.com/mermaid-js/mermaid/archive/8.8.0.zip
$ unzip 8.8.0.zip
$ shasum -a 256 mermaid-8.8.0/dist/mermaid.min.js
d373ecc0e80fed423a6ee72fe30f4307aad5a04d872fa14e98ff3f2e8be4aae0  mermaid-8.8.0/dist/mermaid.min.js

@matalo33 matalo33 added enhancement Improvements to existing features need user feedback labels Sep 10, 2020
@matalo33 matalo33 added this to the v2.6.0 milestone Sep 10, 2020
@mbbx6spp
Copy link
Contributor Author

@matalo33 Sorry for that, I just ran yarn release since that was in the README's instructions for the project and didn't actually check whether a minified file was already present.

Give me a moment to overwrite the file with the official version.

@mbbx6spp
Copy link
Contributor Author

@matalo33 should be fixed now. The SHA256 of the file in the repo at static/mermaid/mermaid.js should now match your SHA.

@matalo33
Copy link
Contributor

Excellent thanks very much!

I confirm d373ecc0e80fed423a6ee72fe30f4307aad5a04d872fa14e98ff3f2e8be4aae0 from dist/mermaid.min.js in https://github.com/mermaid-js/mermaid/archive/8.8.0.zip

@matalo33 matalo33 merged commit 699c9fd into matcornic:master Sep 10, 2020
@mbbx6spp mbbx6spp deleted the mbbx6spp/upgrade-mermaid-v8.8.0 branch September 10, 2020 16:57
@McShelby McShelby mentioned this pull request Sep 13, 2020
@McShelby
Copy link

McShelby commented Sep 17, 2020

Sadly, there are issues with mermaid 8.8.0 and versions prior.

  • Since version 8.2.0, click events and HTML inside of mermaid graphs are only working if securityLevel="loose" is set during mermaid.initialize. See here.

    Since mermaid 8.7.0 there is a securityLevel="antiscript" that seems to be a good fit for a default configuration in our theme.

  • Since mermaid 8.7.0, mermaid.initialize does not carry over its initialized values to a rendered graph. If securityLevel="loose" ist set during our initialization code in footer.html this will not be applied during rendering.

    The official mermaid docs may suffer form the same issue. The <span> tag is renderd as text.

  • The mermaid 8.2.x branch suffers from incorrect padding for some nodes.

I tested mermaid 8.6.4 and all the above issues are not observable there. So that's my current favorite until further issues.

@mirisbowring
Copy link
Contributor

Probably we should add a param to enable / disable your mentioned "fix" and document it on the configuration page.

Since the CDN PR was merged, Users of the CDN will also profit from the toggle fix.

If we just downgrade, users with a later version are still having this Problem.

@McShelby
Copy link

I would prefer to have the whole mermaid config object to be configurable from our config.toml. This would allow users to set further secure mermaid options, that are only settable once during intialization.

This would also allow users to set the mermaid theme, which was requested in #435

I will open another issue and a PR for this.

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

Successfully merging this pull request may close these issues.

4 participants