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

Bump Mermaid Version 9.3.0 -> 10.9.0 #6869

Closed
wants to merge 1 commit into from
Closed

Conversation

Squiddim
Copy link

@Squiddim Squiddim commented May 4, 2024

Hey,

so i bumped the Mermaid Version from 9.3.0 to 10.9.0 to be able to use the new Chart Types.
There have been Breaking Changes in 10.0.0 which i implemented the suggested fix for
(see https://github.com/mermaid-js/mermaid/blob/master/CHANGELOG.md#1000 )
also removed "@types/mermaid since its deprecated and mermaid provides them anyways

I have tested my fix and the only issue i found is that the xychart-beta seems to get cropped a bit. All other charts/diagrams work fine tho

Implement Fixes for Breaking Changes in  Mermaid >10.0.0
@auto-assign auto-assign bot requested a review from tommoor May 4, 2024 00:03
@CLAassistant
Copy link

CLAassistant commented May 4, 2024

CLA assistant check
All committers have signed the CLA.

@tommoor
Copy link
Member

tommoor commented May 4, 2024

This has been attempted several times … if you can get it to work without a massive increase in bundle size then there will be some grateful folks

@Squiddim
Copy link
Author

Squiddim commented May 4, 2024

Not gonna lie this is way above my league ill take a look at the bundle size but if i cant figure it out ill probably end up just building my own image with this fix for now.

@@ -66,7 +66,7 @@ class MermaidRenderer {
const cacheKey = `${isDark ? "dark" : "light"}-${text}`;
const cache = Cache.get(cacheKey);
if (cache) {
element.classList.remove("parse-error", "empty");
elemƒent.classList.remove("parse-error", "empty");
Copy link

@twliqiang twliqiang May 13, 2024

Choose a reason for hiding this comment

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

change the variable name by mistake ?

@willunio
Copy link

I would go as far as to say that outline should remove mermaid support if migrating to 10.x is not going to happen. I've given up with mermaid - everything I do is now in diagrams.net / drawio.

@tommoor
Copy link
Member

tommoor commented May 20, 2024

That really makes no sense, it's not like we're running a years old version – the version of Mermaid in Outline supports all but the latest couple of graph types and is widely used by customers.

@jensschaerer
Copy link

What is the problem with the bundle size? Does an initial load increase from 2,7 MB to 3,3 MB really matter in 2024?

@tommoor
Copy link
Member

tommoor commented May 25, 2024

It adds 33% to the entire size of Outline's client bundle, and 20% to the initial download size for everyone including the majority of users that don't know or care about Mermaid.

Still very much want to enable this upgrade but… yes it matters 😄

@willunio
Copy link

Thank you for continuing to look into it!

Just for those few Mermaid users: the documentation (e.g. for SequenceDiagram) on the Mermaid site is for v10.x, so various things don't work in outline which I expected would. Personally I don't want / need the new diagram types, but it's rather frustrating to try to use Mermaid in Outline right now - you have to find old documentation, then things are missing which I know should be there. Hence my rather radical suggestion to just drop Mermaid completely. As we still have some diagrams in Mermaid/Outline, we'll leave them for now - but slowly we're more likely to use drawio instead, unless we can start relying on Mermaid's documentation.

I can't assess size / clunkiness of implementation. It also annoys me when SW developers are "too lazy" to write efficient code (that's a self-criticism as much as anything...) But I'm afraid it's the world we live in.

@jensschaerer
Copy link

It adds 33% to the entire size of Outline's client bundle, and 20% to the initial download size for everyone including the majority of users that don't know or care about Mermaid.

Still very much want to enable this upgrade but… yes it matters 😄

I don't want to blame you but this is no argument at all - just because it gets bigger doesn't mean it gets slower or unusable at all. of course, if you push this to the limit, it will be the case, but as we are talking about few kilobytes which is no problem for the majority - if not all - devices where I can use Outline, this doesn't matter at all in my opinion.

I'm with @willunio that we can't rely on the documentation and it is frustrating to the user to not be able to see which diagrams are supported. Maybe there is a way to go with an optional "big size build" for "high performance" users or just an online version where the files are used via CDN or static assets instead of delivering the functionality bundled in the client library?

@tommoor tommoor closed this Jun 17, 2024
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.

None yet

6 participants