Skip to content

Conversation

samlaf
Copy link
Collaborator

@samlaf samlaf commented Mar 12, 2025

Why are these changes needed?

Closes EGDA-1073

Currently mdbook mermaid diagrams are not being generated, and look like:
image

This PR fixes this, by using the mdbook-mermaid preprocessor to mdbook.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@samlaf samlaf requested a review from litt3 March 12, 2025 01:34
@samlaf samlaf requested a review from mmurrs March 12, 2025 02:01
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreviewable, gargantuan js files like this make me a bit nervous from a security perspective.

Is it worth having something like this, just to render a few sequence diagrams that could simply be screenshotted for the docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout... there's like 5 diagrams so it would definitely be a bit annoying to have to generate them and keep them updated if we have to copy-paste images. But I guess even with this mdbook-mermaid plugin we're still far from the nice WYSIWYG UI of notion... I think it makes sense to remove this then. @anupsv thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way, as part of build workdlow or something that you can generate the images using the plugin, export it and commit to the PR branch? That way no need to upload the JS file and you don't inherit the hassle of keeping it updated ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I've seen it done in the past is to have a script committed, which uses mermaid cli to generate the diagrams. So, if you make an update to the mermaid file, you just run that script and commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this. Didn't find any way to achieve that result. It would necessitate updating the markdown files replace the mermaid diagrams with image links, and mdbook-mermaid doesn't seem to support that. At least I documented the dependency, pinned mdbook-mermaid to v0.14.1. Also now that the js files are installed they won't ever be updated, so we are trusting that this js file is correct. I looked around and most other projects depend on it as well:

@samlaf samlaf requested review from litt3 and anupsv March 24, 2025 19:57
@samlaf samlaf merged commit 74160cc into master Mar 26, 2025
11 checks passed
@samlaf samlaf deleted the docs--add-mdbook-mermaid-plugin branch March 26, 2025 16:48
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.

3 participants