Skip to content

Add support for mermaid diagrams in README files #6587

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

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

ToBinio
Copy link
Contributor

@ToBinio ToBinio commented Jun 8, 2023

This PR is a Proof of Concept for the in #5724 requested Mermaid support.
It is supposed to show what would be required to support mermaid (maybe I am also missing some major problem with it)

points to think about:

  • should we customize the color theme
  • do we need a stricter securityLevel
  • how could the mermaid.run() call be improved (not using setTimeout())

note: this is just a quick and dirty PoC implementation and should most definitely not be merged

@Turbo87
Copy link
Member

Turbo87 commented Jun 8, 2023

interesting, I would've thought that this would need backend support, but if we can solve it on the frontend this might be viable. since the majority of README files don't need support for it we should try to load the mermaid dependency async though. since the loading of the README content is async already we could potentially put the loading code there and only load it if the README content contains a mermaid reference?

@ToBinio
Copy link
Contributor Author

ToBinio commented Jun 8, 2023

try to load the mermaid dependency

did you mean something like this?

Maby i am missing something but for some reason the code tags of the readme variable have empty class attributes so i don't know a way how to check if it contains some mermaid code.

loadReadmeTask = task(async () => {

@Turbo87
Copy link
Member

Turbo87 commented Jun 8, 2023

did you mean something like this?

very roughly, yes. but we will need to make sure that the loading state stays active until the mermaid dep is loaded and initialized. it might also make sense to have a dedicated Ember.js service to abstract some of this away, so that a) it can be easily mocked in tests and b) we only initialize the library once, instead of for every readme. we will also need to think about how we want to deal with errors in case the async loading fails for some reason (network issues etc.)

@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-frontend 🐹 labels Jun 8, 2023
@ToBinio
Copy link
Contributor Author

ToBinio commented Jun 8, 2023

Maby i am missing something but for some reason the code tags of the readme variable have empty class attributes so i don't know a way how to check if it contains some mermaid code.

I did miss something...
this is only the case for unknown langues

@ToBinio
Copy link
Contributor Author

ToBinio commented Jun 8, 2023

How should we handle a dynamic import error?
do you think it is necessary to re-try or should we just display the unmodified text?

@Turbo87
Copy link
Member

Turbo87 commented Jun 8, 2023

Displaying it unmodified is probably fine in this case, but for the next readme that is displayed we probably should retry it.

@ToBinio
Copy link
Contributor Author

ToBinio commented Jun 8, 2023

would it be ok if I modify the readme-rendering-test.js test? So it also checks for mermaid

module('Acceptance | README rendering', function (hooks) {

@Turbo87
Copy link
Member

Turbo87 commented Jun 8, 2023

sure, but probably makes sense to have dedicated tests for this and keep the existing ones as they are.

@ToBinio
Copy link
Contributor Author

ToBinio commented Jun 9, 2023

Do you think we should change the securitylevel to sandBox? Tbh I am not completely sure what it changes. (I couldn't find anything)

About the color theme. I don't think it looks very off, so I am not sure if it is necessary to go through all of the options to customize them right now.
image

another thing that should be done at some point would be to add some sort of zoom/scroll possibility for big graphs like in natural-xml-diff

is there anything left I missed and still needs to change?

@GreenerInit

This comment was marked as spam.

@bors
Copy link
Contributor

bors commented Jun 19, 2023

☔ The latest upstream changes (presumably d169e34) made this pull request unmergeable. Please resolve the merge conflicts.

@ToBinio ToBinio marked this pull request as ready for review June 20, 2023 13:12
@ToBinio ToBinio changed the title PoC: mermaid support mermaid support Jun 20, 2023
@Turbo87 Turbo87 changed the title mermaid support Add support for mermaid diagrams in README files Jun 21, 2023
@Turbo87 Turbo87 requested a review from a team June 21, 2023 14:46
@Turbo87
Copy link
Member

Turbo87 commented Jun 21, 2023

@ToBinio I've rewritten the JS portion of this a little bit using an Ember.js "modifier". In the controller we try to load the library, and in the modifier we will use the library (if it has been loaded) to render the diagrams in the README.

I'll keep this PR open a for bit longer to let others chime in if they have any concerns with this. I've also enabled the sandbox mode to ensure we don't open any XSS vectors on crates.io.

@LawnGnome
Copy link
Contributor

There's a relevant discussion in the docs.rs Zulip stream.

I don't think exact feature parity between crates.io and docs.rs for README rendering is a hard requirement, but I do wonder if the frontend-only approach is the right one here.

@Turbo87
Copy link
Member

Turbo87 commented Jun 21, 2023

but I do wonder if the frontend-only approach is the right one here

I'm open to other alternatives...

The Zulip stream mentioned that there are no Rust-based mermaid impls though, so I'm not sure what else to use 🤔

@ToBinio
Copy link
Contributor Author

ToBinio commented Jun 21, 2023

This is probably just one of my naive ideas. But I just discovered the mermaid.ink project. Which has an API to render mermaid graphs to images.

in theory, this could be used.
but somebody should definitely talk with the maintainers because this would most likely significantly increase their server load.

something similar could also be self-hosted but this would increase the complexity immensely and probably not be reasonable

@LawnGnome
Copy link
Contributor

The Zulip stream mentioned that there are no Rust-based mermaid impls though, so I'm not sure what else to use thinking

We discussed this at the team meeting this week, and I feel better about this after the discussion there, particularly since we don't have a viable backend-only alternative right now (not counting mdbook-mermaid, which just shells out to node).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants