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

Pin to Mermaid v9 #254

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Pin to Mermaid v9 #254

merged 1 commit into from
Nov 8, 2023

Conversation

seshrs
Copy link
Member

@seshrs seshrs commented Nov 8, 2023

Context

Closes eecs280staff/p5-ml#183.

@amirkamil noticed that Mermaid diagrams weren't rendering on the P5 spec:

Expected Actual
Screenshot 2023-11-06 at 8 15 52 PM Screenshot 2023-11-06 at 8 14 11 PM

Digging a bit, I found mermaid-js/mermaid#4442, which noted that the root cause was a breaking change in v10 of MermaidJS:

Mermaid is ESM only!

I didn't notice when adding MermaidJS support to Primer Spec, but it turned out that the documentation had let me to always use the latest version of MermaidJS instead of pinning to a specific version — hence, we were exposed to the breaking change :(

In this PR

This commit changes the Primer Spec to always use MermaidJS v9, which is compatible with CJS and exposes the global variable mermaid.

In future, it would make sense to follow-up how best to upgrade to Mermaid v10. But this works for now.

Validation

Visit the preview URL's "Diagrams" demo. Notice that the Mermaid diagrams now render again.

@seshrs seshrs added the semver/minor Pull Request proposes "minor" change label Nov 8, 2023
Copy link
Contributor

github-actions bot commented Nov 8, 2023

The spec from this PR is available at https://preview.sesh.rs/previews/eecs485staff/primer-spec/254/.

(Available until Fri Dec 08 2023.)

@seshrs seshrs merged commit 9ea9afa into develop Nov 8, 2023
3 checks passed
@seshrs seshrs deleted the mermaid-fix branch November 8, 2023 06:31
@awdeorio
Copy link
Contributor

awdeorio commented Nov 8, 2023

In future, it would make sense to follow-up how best to upgrade to Mermaid v10. But this works for now.

If you feel this is something that's important to track, consider making an issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Pull Request proposes "minor" change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants