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

Create Mermaid live editor preview for PRs #3348

Closed
dbartholomae opened this issue Aug 23, 2022 · 4 comments
Closed

Create Mermaid live editor preview for PRs #3348

dbartholomae opened this issue Aug 23, 2022 · 4 comments
Labels
Status: Triage Needs to be verified, categorized, etc Type: Enhancement New feature or request

Comments

@dbartholomae
Copy link
Contributor

Is your feature request related to a problem? Please describe.
When developing new features for mermaid itself, it can be hard to review them easily, as there is no easy way to just play around with Mermaid diagrams in the browser. This discussion came up for the C4 integration.

Describe the solution you'd like
Ideally, each PR creates a custom deployment of the Mermaid Live Editor, using the PRs version of Mermaid. This could be achieved with a service like Netlify that is already used to deploy the production version of the Mermaid Live Editor.

The biggest challenge is to get the correct mermaid version to be used by the Live Editor. In the current form, the Live Editor uses the npm repository to install mermaid itself.
One solution to this problem could be to move the project into a monorepo, and then to use the local git version of Mermaid instead of the npm version. This could be facilitated with a monorepo management tool like rush or turborepo.

Describe alternatives you've considered
The simplest alternative to a live editor would be running mermaid PRs locally with help of the e2e setup, but this would be significantly more work for a reviewer compared to just clicking a link in the PR.

As an alternative to using a monorepo, we could change the Live Editor to load mermaid asynchronously with an option to select the source, and publish a mermaid version for each PR. This would be significantly more work than moving the projects to a monorepo.

Instead of Netlify, we could also use other services like e.g. Amplify, but since the editor already uses Netlify, this might be additional work without additional benefits.

Additional context
I'm happy to help around implementation. I've already moved open source projects to monorepos and have experience specifically with rush.

@dbartholomae dbartholomae added Status: Triage Needs to be verified, categorized, etc Type: Enhancement New feature or request labels Aug 23, 2022
@aloisklink
Copy link
Member

aloisklink commented Aug 25, 2022

One solution to this problem could be to move the project into a monorepo, and then to use the local git version of Mermaid instead of the npm version. This could be facilitated with a monorepo management tool like rush or turborepo.

I'm slightly against moving mermaid into a mono-repo.

Currently, people can use npm install git+https://github.com/mermaid-js/mermaid or yarn add git+https://github.com/mermaid-js/mermaid to use the latest git version of mermaid in their project.

(they can also fork mermaid and use git+https://github.com/username-here/mermaid or git+https://github.com/username-here/mermaid#my-branch-here)

Unfortunately, npm/yarn git urls don't yet support mono-repos.

As an alternative to using a monorepo, we could change the Live Editor to load mermaid asynchronously with an option to select the source, and publish a mermaid version for each PR. This would be significantly more work than moving the projects to a monorepo.

Every PR's Build action also creates a dist.zip file already, so it should be pretty easy to uncompress it to the mermaid.min.js (or upload that separately)

Alternatives (in my opinion)

We could make a GitHub action that does a:

git clone https://github.com/mermaid-js/mermaid-live-editor
cd mermaid-live-editor
yarn add ${{ github.event.head_repository.html_url}}#${{ github.event.head_sha }}
yarn install
yarn build

but I've got no idea if Netlify will support this for PRs.

Even better (visual inspection tests)

Edit: Nevermind, I'm dumb. There's already visual inspection tests with imgSnapshotTest 🤦. That's my fault for not looking hard enough.

Alternatively, instead of manually testing with something like Netlify, we could add automatic visual tests. This would be even better, since it will ensure making a change doesn't change other diagrams. Never mind, I'm dumb

@sidharthv96
Copy link
Member

yarn add ${{ github.event.head_repository.html_url}}#${{ github.event.head_sha }}

Seems like the best option without making any breaking changes, keeping things simple.

@dbartholomae
Copy link
Contributor Author

dbartholomae commented Aug 28, 2022

@sidharthv96 not sure this would work - the git does not contain the built version, only the source.

EDIT: Just saw the prepare script, so it would only be a problem for those who disabled these scripts for security reasons. So this seems to be the most promising way :)

Even better (visual inspection tests)

@aloisklink This doesn't actually solve the problem, as you can make sure that things don't break, but you can't do exploratory testing.

@aloisklink
Copy link
Member

aloisklink commented Sep 7, 2023

Implemented by #4769!

The approach isn't exactly the same (since Mermaid is now a mono-repo), but it seems to work pretty well :) Thanks @sidharthv96 for implementing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage Needs to be verified, categorized, etc Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants