Replace `dagre`/`dagre-d3` with `dagre-d3-es` #3809

aloisklink · 2022-11-18T02:17:14Z

📑 Summary

Replace the dagre and dagre-d3 libraries with dagre-d3-es.

Both dagre and dagre-d3 are deprecated and unmaintained, and haven't been updated for more than 3 years.

Since dagre-d3 still requires an old version of d3, this causes a bunch of security warnings, e.g. GHSA-36jr-mh4h-2g58

The dagre-d3-es package is a fork that contains support for "d3": "^7.6.1". Also, it's ESM, so we will get smaller bundle sizes too (mermaid.esm.min.mjs seems to be about 180 KB smaller!)

The only issue is that this fork isn't very well used (only has 3000 weekly downloads), compared to dagre-d3's 250,000 weekly downloads (although to be fair, a large proportion of dagre-d3's downloads probably come from mermaid).

Resolves #3712
Resolves #3705 (there might be other issues, but pnpm audit now says No known vulnerabilities found)
Resolves #3666
Resolves #2626

📏 Design Decisions

The mindmap e2e tests were being a bit flaky.

Sometimes, the mindmap e2e tests take a snapshot when the mindmap SVG has been created, but hasn't yet been fully rendered. E.g., the <svg ....> exits, but is empty.

I've added a quick check for a mindmap section root, so that the snapshot is only taken after the mindmap diagram has started rendering, and it seems like nothing has changed in the e2e tests.

I also added a demos/mindmap.html example, since I needed it anyway to help write the Cypress E2E tests.

Since it's is a less popular package, I've pinned dagre-d3-es to "7.0.2" instead of "^7.0.2". This does mean if there is a bug in dagre-d3-es, we will have to manually bump it ourselves, but it also means we won't accidentally be sending a buggy version of dagre-d3-es out to users in cases something changes (it might be worth disabling renovate for this if we're feeling paranoid!)

In the long-term, we should slowly move towards removing dagre if we can, see #1969.

📋 Tasks

Make sure you

📖 have read the contribution guidelines
💻 have added unit/e2e tests (if appropriate)
🔖 targeted develop branch

sidharthv96

The ~200KB decrease in size is phenomenal!!

sidharthv96 · 2022-11-18T03:51:34Z

packages/mermaid/src/diagrams/flowchart/flowRenderer.js

-import addHtmlLabel from 'dagre-d3/lib/label/add-html-label.js';
+import { render as Render } from 'dagre-d3-es';
+import { applyStyle } from 'dagre-d3-es/src/dagre-js/util';
+import { addHtmlLabel } from 'dagre-d3-es/src/dagre-js/label/add-html-label.js';


Is the .js extension necessary?

It is normally required in the browser/Node.JS, see https://nodejs.org/api/esm.html#mandatory-file-extensions, but it looks like Vite is allowing it.

My gut feeling is to add the .js extension to everything except for things exported directly by the package.json. I've changed this in fd76e0e (I've git rebase-d my commits).

We could also try to add https://nodejs.org/api/packages.html#subpath-patterns to dagre-d3-es, so that we could do stuff like import {applyStyle} from 'dagre-d3-es/dagre-d3/util.js (e.g. without the /src/ bit) instead, but even then, the Node.JS docs seem to recommend only exporting paths that end with .js if possible.

demos/mindmap.html

cypress/integration/rendering/mindmap.spec.js

Currently, we have mindmap tests in the cypress/integration/rendering/mermaid.spec.js which is a bit odd. They should probably all be in the mindmap.spec.js file.

All tests have been moved to `mindmap.spec.js` in a previous commit.

Sometimes, the mindmap e2e tests take a snapshot when the mindmap SVG has been created, but hasn't yet been fully rendered. This adds a quick check for a mindmap section root, so that the snapshot is only taken after the mindmap diagram has started rendering. I was also running into JSDoc ESLint warnings, so I moved the file into a TypeScript file to fix those warnings.

Replace the dagre and dagre-d3 libraries with dagre-d3-es. Both dagre and dagre-d3 are deprecated and unmaintained, and haven't been updated for more than 3 years. Since dagre-d3 still requires an old version of d3, this causes a bunch of security warnings, e.g. GHSA-36jr-mh4h-2g58 The [dagre-d3-es](https://github.com/tbo47/dagre-es) package is a fork that contains support for `"d3": "^7.6.1"`. Also, it's ESM, so we will hopefully get smaller bundle sizes too. The only issue is that this fork isn't very well used (only has 3000 weekly downloads), compared to `dagre-d3`'s 250,000 weekly downloads. (although to be fair, a large proportion of dagre-d3's downloads probably come from mermaid) Since it's is a less popular package, **I've pinned `dagre-d3-es` to `"7.0.2"` instead of `"^7.0.2"`**. This does mean if there is a bug in `dagre-d3-es`, we will have to manually bump it ourselves, but it also means we won't accidentally be sending a buggy version of `dagre-d3-es` out to users in cases something changes (it might be worth disabling renovate for this if we're feeling paranoid!)

aloisklink · 2022-11-18T18:54:56Z

The ~200KB decrease in size is phenomenal!!

I think we can go further actually :)

dagre-d3-es has it's own version of graphlib in it, see https://github.com/tbo47/dagre-es/tree/main/src/graphlib. And graphlib is like a 500 KiB library, so using an ESM version might greatly decrease our bundle size. I didn't put it into this PR, since it looks like graphlib still has some activity (even if there hasn't been an NPM release for 3 years +), but it could be a future PR.

We could also try swapping out lodash for lodash-es and see if that decreases bundle size.

sidharthv96 · 2022-11-19T17:31:07Z

I was experimenting with that yesterday, analyzing our bundle size. Already replaced lodash and it did make a difference. I've also split out our internal diagrams from the bundle.

weedySeaDragon · 2022-11-19T18:06:24Z

@sidharthv96 - what tool(s) did you use to generate the bundle size info? ( == my lack of experience with dealing with js toolchain)

aloisklink · 2022-11-19T18:28:15Z

I've also split out our internal diagrams from the bundle.

I think we should keep all the internal diagrams in a single bundle, mainly for backwards compatibility.

In the mermaid-cli project, we used to just copy the mermaid.min.js file and use that (it's part of the reason why we had so much trouble switching to using mindmaps). A lot of other projects seem to do the same thing, they just copy the bundle over into their static website.

To be honest, we don't really save much by moving it into separate bundles anyway, because all of the internal diagrams are always imported anyway by default (unless we enable lazy loading for all of them too).

E.g.

IFFE single file, no other dependencies (not even the ones in node_modules): mermaid.min.js
ESM single file, no other dependencies (not even the ones in node_modules): mermaid.esm.min.mjs
ESM import (listed in package.json): can be anything, because this assumes that a downstream project (Webpack/Vite/TypeScript/ESBuild/rollup) will bundle Mermaid into somebody else's app/website
CommonJS require (listed in package.json): just points to mermaid.min.js, because we have to bundle ESM dependencies for them to work in CommonJS. Only here for backwards compatibility.

sidharthv96 · 2022-11-20T05:35:40Z

To be honest, we don't really save much by moving it into separate bundles anyway, because all of the internal diagrams are always imported anyway by default (unless we enable lazy loading for all of them too).

I did enable lazy loading for them. There are some kinks to iron out, but there is a performance difference.
If we bundle all the detectors, then the request count and loading time will come down further.

Good 3G

mermaid.min.js

Unbundled

4G

mermaid.min.js

Unbundled

WiFi

mermaid.min.js

Unbundled

After moving styles out,

sidharthv96 · 2022-11-20T05:37:35Z

@sidharthv96 - what tool(s) did you use to generate the bundle size info? ( == my lack of experience with dealing with js toolchain)

@weedySeaDragon I used https://github.com/btd/rollup-plugin-visualizer. Which has very nice diagrams.
The network diagram is a bit too messy, though.

Image from their readme.

aloisklink · 2022-11-20T16:19:02Z

I did enable lazy loading for them. There are some kinks to iron out, but there is a performance difference.
If we bundle all the detectors, then the request count and loading time will come down further.

🚀 Those size/speed differences look pretty good!

I still think that the bundles the Mermaid project provides shouldn't change (at least unless it's announced as a breaking change in a v10), but enabling lazy loading for all the diagrams does make sense, since then people using mermaid with their own bundlers will get the performance benefits!

tbo47 · 2022-11-20T19:04:01Z

I just released dagre-d3-es v7.0.4: https://www.npmjs.com/package/dagre-d3-es

It includes the modification with intersect (that you are not using) and the loadash optimization
https://github.com/tbo47/dagre-es/pulls?q=is%3Apr+is%3Aclosed

aloisklink added the dependencies label Nov 18, 2022

aloisklink mentioned this pull request Nov 18, 2022

Update packages to fix vulnerabilities #3712

Closed

sidharthv96 approved these changes Nov 18, 2022

View reviewed changes

aloisklink added 5 commits November 18, 2022 17:32

docs: create some basic mermaid mindmap demos

6e234f1

test(e2e): move mindmap tests to mindmap.spec.js

b9dcdb0

Currently, we have mindmap tests in the cypress/integration/rendering/mermaid.spec.js which is a bit odd. They should probably all be in the mindmap.spec.js file.

test(e2e): remove unused mermaid.spec.js file

57edcfe

All tests have been moved to `mindmap.spec.js` in a previous commit.

aloisklink force-pushed the use-dagre-d3-es branch from d708ed8 to fd76e0e Compare November 18, 2022 18:46

sidharthv96 approved these changes Nov 20, 2022

View reviewed changes

mmorel-35 approved these changes Nov 20, 2022

View reviewed changes

sidharthv96 mentioned this pull request Nov 20, 2022

Reduce mermaid size by 31% #3825

Merged

3 tasks

knsv merged commit 29a02d1 into mermaid-js:develop Nov 21, 2022

aloisklink deleted the use-dagre-d3-es branch November 21, 2022 23:54

olayway mentioned this pull request Nov 22, 2022

Investigate high severity vulnerabilities datopian/flowershow#200

Closed

MgenGlder mentioned this pull request Dec 1, 2022

Deploying an update to address security vulnerability #3877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `dagre`/`dagre-d3` with `dagre-d3-es` #3809

Replace `dagre`/`dagre-d3` with `dagre-d3-es` #3809

aloisklink commented Nov 18, 2022

sidharthv96 left a comment

sidharthv96 Nov 18, 2022

aloisklink Nov 18, 2022

aloisklink commented Nov 18, 2022

sidharthv96 commented Nov 19, 2022

weedySeaDragon commented Nov 19, 2022

aloisklink commented Nov 19, 2022 •

edited

Loading

sidharthv96 commented Nov 20, 2022 •

edited

Loading

sidharthv96 commented Nov 20, 2022

aloisklink commented Nov 20, 2022 •

edited

Loading

tbo47 commented Nov 20, 2022

Replace dagre/dagre-d3 with dagre-d3-es #3809

Replace dagre/dagre-d3 with dagre-d3-es #3809

Conversation

aloisklink commented Nov 18, 2022

📑 Summary

📏 Design Decisions

📋 Tasks

sidharthv96 left a comment

Choose a reason for hiding this comment

sidharthv96 Nov 18, 2022

Choose a reason for hiding this comment

aloisklink Nov 18, 2022

Choose a reason for hiding this comment

aloisklink commented Nov 18, 2022

sidharthv96 commented Nov 19, 2022

weedySeaDragon commented Nov 19, 2022

aloisklink commented Nov 19, 2022 • edited Loading

sidharthv96 commented Nov 20, 2022 • edited Loading

Good 3G

4G

WiFi

sidharthv96 commented Nov 20, 2022

aloisklink commented Nov 20, 2022 • edited Loading

tbo47 commented Nov 20, 2022

Replace `dagre`/`dagre-d3` with `dagre-d3-es` #3809

Replace `dagre`/`dagre-d3` with `dagre-d3-es` #3809

aloisklink commented Nov 19, 2022 •

edited

Loading

sidharthv96 commented Nov 20, 2022 •

edited

Loading

aloisklink commented Nov 20, 2022 •

edited

Loading