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

Add mermaid.tiny.min.js. 69.7% size reduction. #4734

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

sidharthv96
Copy link
Member

📑 Summary

mermaid.min.js: 3mb
mermaid.tiny.min.js: 1.1mb

Resolves #4616

image image

📏 Design Decisions

Uses the define feature of esbuild to exclude mindmap and flowchart-elk during build time.

📋 Tasks

Make sure you

Excludes elk and mindmap at build time
* sidv/splitChunks:
  chore: Add analyzer comment
  chore: Split chunks into folders
  chore: Split chunks into folders
  chore: Add defaultOptions to server
  chore: Split chunks into folders
@sidharthv96 sidharthv96 changed the base branch from develop to sidv/esbuildV11 August 14, 2023 03:18
@sidharthv96 sidharthv96 changed the base branch from sidv/esbuildV11 to sidv/splitChunks August 14, 2023 03:18
@sidharthv96 sidharthv96 marked this pull request as draft August 14, 2023 03:19
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Attention: Patch coverage is 0% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 5.72%. Comparing base (3809732) to head (70038e9).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #4734      +/-   ##
==========================================
- Coverage     5.72%   5.72%   -0.01%     
==========================================
  Files          278     278              
  Lines        42013   42046      +33     
  Branches       490     490              
==========================================
  Hits          2407    2407              
- Misses       39606   39639      +33     
Flag Coverage Δ
unit 5.72% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ackages/mermaid/src/diagrams/error/errorDiagram.ts 0.00% <0.00%> (ø)
packages/mermaid/src/mermaidAPI.ts 0.00% <0.00%> (ø)
.vite/build.ts 0.00% <0.00%> (ø)
...s/mermaid/src/diagram-api/diagram-orchestration.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/info/infoRenderer.ts 0.00% <0.00%> (ø)
.esbuild/build.ts 0.00% <0.00%> (ø)
.esbuild/util.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/common/common.ts 52.33% <0.00%> (-0.97%) ⬇️

@sidharthv96 sidharthv96 marked this pull request as ready for review August 14, 2023 03:43
@sidharthv96 sidharthv96 changed the title Add mermaid.tiny.min.js Add mermaid.tiny.min.js. 69.7% size reduction. Aug 14, 2023
@sidharthv96
Copy link
Member Author

image

The version will be shown in error and info diagrams.

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Nice! That's a massive size reduction!

I'm really against putting this in the mermaid package, because it will make the package bigger for everybody, even if they don't use the mermaid.tiny.min.js file, and it will add another file that we will never be able to get rid of without breaking backwards compatibility with CDNs.

I think this must go in a new package, maybe something like @mermaid-js/mermaid-tiny?


Two other recommendations that would be nice to fix, but not required:

  • Instead of changing src/diagrams/.../detector.ts, can't we just prevent adding mindmap and flowchart/elk in the the diagram-orchestration.ts file, e.g. modifying these lines of code:

    flowchartElk,
    flowchartV2,
    flowchart,
    mindmap,

    If ESBuild is smart enough, it should do tree shaking automatically, and it will have the same effect.

  • I think the error diagram needs a more human readable message in case somebody does try to use mindmap or flowchart-elk with mermaid-tiny. We have to assume that users might not even know they're using mermaid-tiny, e.g. if they're using a third-party Mermaid plugin.

@sidharthv96
Copy link
Member Author

sidharthv96 commented Aug 16, 2023

I'm really against putting this in the mermaid package, because it will make the package bigger for everybody, even if they don't use the mermaid.tiny.min.js file.

The current unpacked size is 21MB, adding an extra 1MB to it is worth the tradeoff for not having to maintain and release another package. Moreover, this will only affect people using package managers, not actual end users.
Also, the compressed size increase would be few hundred KBs, which is negligible for our usecase.

Version Size Compressed
8.0.0 17.7 MB 3.4 MB
9.0.0 23.8 MB 5 MB
10.0.0 32.9 MB 6.5 MB
10.3.1 21.3 MB 4.5 MB
11.0.0-alpha.4 14.7 MB 4.2 MB

It will add another file that we will never be able to get rid of without breaking backwards compatibility with CDNs.

Now it's just a file, if we use a package, we'll have to maintain that in sync with mermaid, which is a bigger headache than having a file in the package with 0 active maintenance requirement.

@sidharthv96
Copy link
Member Author

sidharthv96 commented Aug 16, 2023

Instead of changing src/diagrams/.../detector.ts, can't we just prevent adding mindmap and flowchart/elk in the the diagram-orchestration.ts file, e.g. modifying these lines of code:

I tried that first, the file size didn't change. But I was using a different approach then (not define). Let me try now.

--

It worked! ❤️
This simplifies it a lot.

sidharthv96 and others added 2 commits August 16, 2023 11:01
Co-authored-by: Alois Klink <alois@aloisklink.com>
@sidharthv96
Copy link
Member Author

sidharthv96 commented Aug 16, 2023

I think the error diagram needs a more human readable message in case somebody does try to use mindmap or flowchart-elk with mermaid-tiny. We have to assume that users might not even know they're using mermaid-tiny, e.g. if they're using a third-party Mermaid plugin.

That would be out of scope for this PR. I did try a POC, but the changes will be extensive.

Especially since we're not even registering the diagrams since 55266c4

@sidharthv96 sidharthv96 added this to the v11 milestone Aug 16, 2023
@aloisklink
Copy link
Member

It will add another file that we will never be able to get rid of without breaking backwards compatibility with CDNs.

Now it's just a file, if we use a package, we'll have to maintain that in sync with mermaid, which is a bigger headache than having a file in the package with 0 active maintenance requirement.

I still don't agree with adding the mermaid.tiny.min.js file to the mermaid NPM package. It is something we might be stuck with forever, so I think it will actually add maintenance overhead. (I'm a bit worried that if Mermaid is still around 100 years in the future, this mermaid.tiny.min.js file will still be there, and we won't be able to get rid of it).

Would uploading this as a GitHub Release asset be a better idea (e.g. using something like https://docs.github.com/en/rest/releases/assets?apiVersion=2022-11-28)? This should be pretty easy to do (we just need to add another job in https://github.com/mermaid-js/mermaid/blob/develop/.github/workflows/release-publish.yml). Since GitHub Releases don't have an @latest feature, we'd then be safe to remove it/change it if we ever want to.

Regardless of what we pick, since adding another entry-point to mermaid is a pretty major change, we should get @knsv's approval first (maybe also other mermaid maintainers?).

I think long-term, having it in a separate NPM package like @mermaid-js/mermaid-tiny would be the cleanest way of doing things, but you're right, there's no point in doing that now until we fully migrate the mermaid repo into a mono-repo, with automated releases/changelogs for each individual package.


That would be out of scope for this PR. I did try a POC, but the changes will be extensive.

That's fair! It would be a nice feature to have, especially if custom Mermaid plugins become more popular (although since it's not a breaking change, that would be a feature that can go directly into the develop branch!)

@sidharthv96
Copy link
Member Author

Would uploading this as a GitHub Release asset be a better idea (e.g. using something like https://docs.github.com/en/rest/releases/assets?apiVersion=2022-11-28)? This should be pretty easy to do (we just need to add another job in https://github.com/mermaid-js/mermaid/blob/develop/.github/workflows/release-publish.yml). Since GitHub Releases don't have an @latest feature, we'd then be safe to remove it/change it if we ever want to.

This could be a stop-gap measure till we have automated builds and separate packages.

Also, I don't think the @latest would be a major issue for us, as we have replaced the documentation with the correct major version number. So only people who go out of their way to use @latest against the official documentation would be affected when/if we remove the file from a later version of mermaid. As long as we keep it in v11, it should be good.

@sidharthv96
Copy link
Member Author

sidharthv96 commented Aug 17, 2023

I have this idea for fully customizable mermaid that people can download, containing code for only the diagrams that they need, in a single bundle, without lazy loading.

A browser based version could be done using esbuild-wasm, once the bundling in browser part is figured out.

@sidharthv96 sidharthv96 changed the base branch from sidv/splitChunks to next August 17, 2023 06:47
* next:
  chore: Move SVG import to comment.
  build docs
  Remove whitespace on empty line
  chore: Fix minify
  Documentation for #2509
  Update all minor dependencies
  Update all patch dependencies
  make more `RectData` required and remove optional assignment
  use lineBreakRegex in `svgDrawCommon`
  fix svgDrawCommon import by adding `.js`
  add types to `svgDrawCommon.ts`
  convert `svgDrawCommon` to TS
@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 70038e9
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/661e23172c6de70008205fb4
😎 Deploy Preview https://deploy-preview-4734--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

* next: (27 commits)
  chore: Fix type
  refactor!: remove MermaidConfig type enum fallback
  test: rewrite some `config` vals to tighten types
  chore: Add comment for `yy`.
  chore: Increase heap size when building
  chore: increase `test-util.ts` converage by returning `undefined`
  chore: add `vitest` imports to `test-util.ts`
  chore: run `pnpm lint:fix`
  create `noErrorsOrAlternatives` parser helper function
  chore: export `InfoModule` from `infoModule.ts`
  docs(parser): create `packages/parser` README.md file
  build: build `.langium` file using `generate` from `langium-cli`
  build: update `langium` and `langium-cli` to `v2.0.1`
  fix: fix if statment logic checks if `parser` is not `undefined`
  chore: add a comment illustrate why we build packages sequentially
  chore: refactore `&&` into `if` in `populateCommonDb`
  chore: remove `./*` part from `exports` in `parser/package.json`
  fix: use `execFileSync` instead of `execSync` in `generateLangium`
  fix(mermaid): mark `mermaid-parser` dependecy with `^`
  reorder `packages/parser` after `packages/mermaid/src/vitepress`
  ...
@spong
Copy link

spong commented Nov 16, 2023

Hey there @sidharthv96 and @aloisklink!

Thank you so much working this min variant. We're looking to add Mermaid support to our markdown plugin within the Elastic Assistant (elastic/kibana#167723 -- our testing of llm generated mermaid charts has been awesome!) however due some licensing issues around elkjs it looks like consuming this min variant without it is the best path forward at the moment.

It seems there's still some discussion going on WRT packaging, but is there anything we could contribute here to help get this effort in? Happy to help where we can. 🙂 Thanks again!

@aloisklink
Copy link
Member

It seems there's still some discussion going on WRT packaging, but is there anything we could contribute here to help get this effort in? Happy to help where we can. 🙂 Thanks again!

Hi @spong, I'm not sure if this exact PR will get merged anytime soon, but we are planning on removing elkjs from the default mermaid package in #5049, since quite a few people have raised issues about the license, and we believe it would be backwards compatible (it would just be a visual change).

If it gets merged, it should be in the next major/v11 release, which should hopefully come out before the Christmas/New Year holiday period!

It's not the same as this PR, as mindmaps will still be included, which has a dependency on the very large cytoscape package, but that package is MIT licensed, so there shouldn't be any licensing issues there.

@spong
Copy link

spong commented Nov 20, 2023

Oh awesome -- thank you so much for the details @aloisklink! From a packaging perspective we're definitely still interested in a min variant, but that's not a strict blocker for adoption, so we will look forward to integrating once v11 is released.

Appreciate all your work here, we're big fans of mermaid and can't wait to introduce it to more folks 🙂

@sidharthv96
Copy link
Member Author

@spong can you verify if https://www.npmjs.com/package/mermaid/v/11.0.0-alpha.5 complies with the license requirements? We've removed elkjs.

@patrykkopycinski
Copy link

Thank you @sidharthv96 for taking care of that so quickly 🙇
Unfortunately, it seems we still have an issue with mermaid-parser and dompurify (this should be good if we adjust our config) 😞
image

@sidharthv96
Copy link
Member Author

mermaid-parser is MIT licenced. https://github.com/mermaid-js/mermaid/blob/next/packages/parser/LICENSE

And DOMPurify is Apache/MPL.

@patrykkopycinski
Copy link

patrykkopycinski commented Nov 26, 2023

@sidharthv96
Copy link
Member Author

sidharthv96 commented Nov 26, 2023 via email

@patrykkopycinski
Copy link

Awesome, Thank you! 🙇

@sidharthv96
Copy link
Member Author

sidharthv96 commented Nov 26, 2023

@patrykkopycinski https://www.npmjs.com/package/@mermaid-js/parser is out with MIT license, available in mermaid v11.0.0-alpha.6.

@patrykkopycinski
Copy link

Hey @sidharthv96, I just wanted to check if there is any news about the v11 release?

* next: (562 commits)
  Lint
  Remove echo
  RefTest
  Echo event
  Update cypress
  Fix applitools
  Fix applitools
  docs: fix lint
  docs: move community to Discord
  Swap condition blocks to avoid using negation
  Reposition const declaration to ideal place
  Change repetitive values into consts
  docs: fix swimm link
  Fix Update Browserslist
  Use pnpm/action-setup@v2
  Fix lint
  Cleanup e2e.yml
  Ignore push events on merge queue
  Remove ::
  Remove ::
  ...
@sidharthv96
Copy link
Member Author

sidharthv96 commented Jan 24, 2024

With the removal of elk js and addition of the new parser, the size delta has decreased significantly. It's 2.2 vs 1.7 MB now.

image image

See eclipse-langium/langium#1168 for Langium's size reduction efforts.

image

* next: (269 commits)
  Resolves E2E testing issues and issue #5343
  Fix spelling
  Fix community integrations
  Fix docs
  docs: Fix config
  Update all minor dependencies
  Amend docs to document gitgraph parallel commits
  Fix lint
  Use Yarn Add COREPACK_ENABLE_STRICT
  Added link to Blazorade Mermaid to the community integrations page.
  Bump node version
  Add lcov to cspell
  Correcting path to docker-entrypoint.sh
  Update recommended Vitest extension
  Replace mermaid-js.github.io links
  Replace links to docs with links to webhelp
  Link to contributing page on webhelp
  Changes to timeline.md 1. Added colons to all 'NOTES' for consistency.
  Changes to timeline.md 1. Updates the Wikipedia citation to include a link. 2. Removed periods from documentation sections to be consistent (some had periods, some didn't) 3. Added a space to a coding example for spacing consistency.
  Replace version number placeholder
  ...
Base automatically changed from next to develop March 6, 2024 09:23
sidharthv96 and others added 3 commits March 8, 2024 19:43
* develop: (31 commits)
  make LLM integrations a new headline
  build docs
  ci(e2e): avoid commenting on PRs in CI
  chore: Remove unused imports in block
  Fix spelling
  Update docs
  Lychee ignore chrome webstore
  Update link
  chore(deps): update all patch dependencies
  build(docs): vendor CSS dependencies
  chore(deps): update all minor dependencies
  fix linting issues
  Ran lint:fix
  Fix chrome webstore url causing 404
  Add LLM integration section in productivity tools and add HueHive
  build(deps): update `langium` to `v3` and apply the required changes
  Remove dummy change from e2e.yml
  Remove dummy change
  Remove log
  Format message
  ...
* develop: (101 commits)
  Add extra test
  Add visual test
  Wait for image to be rendered
  Update docs
  Linting
  chore:  temp fix for eslint OOM
  chore: Update error snapshots
  Fix ESLint
  chore: Prettier
  chore: YOLO `pnpm --recursive update`
  chore: Remove commitlint
  Fix flowchart-elk render test
  chore: Add example page link in index
  Fix flowchart-elk render test
  chore: Add example page link in index
  fix: Remove space which caused extra newline on diagrams
  fix: Remove repeated config calls
  fix: ELK diagram remove re-parsing
  chore: Minor fixes #4856
  chore: Increase ESLint memory limit
  ...
@Dhoot
Copy link

Dhoot commented Apr 16, 2024

@sidharthv96 is there any way we can use mermaid.tiny ?

* develop: (52 commits)
  docs: Add quadrant point styling
  feat: Change precedence of styling
  chore(deps): update all minor dependencies
  chore(deps): update all patch dependencies
  fix: eslint ignore, type definition
  chore(deps): update all patch dependencies
  fix: Remove `ImperativeState` type restriction.
  📝🐛 fix schema link
  update latest news section
  Changes to rendering/gitGraph.spec.js - Added additional rendering test functionality for recognizing 'switch' as an alias to 'checkout'.
  1. Changes to gitGraph.jison - Updated the regex to allow either 'checkout' or 'switch' 2. Changes to gitGraphParser.spec.js - Additional test coverage added for the changes made to the parser. 3. Changes to gitGraphParserV2.spec.js - Additional test coverafe added for the changes made to the parser. 4. Changes to gitgraph.md - Updated documentation to let users know that checkout/switch can be used interchangeably.
  revert from and to type to object
  add eslint rule consistent-type-definations
  Update createText.ts
  chore(deps): update all patch dependencies
  revert lock file
  simplify message type from and to
  move types to separate file
  use interfaces instead of types
  feat: create utils func + test cases
  ...
@sidharthv96
Copy link
Member Author

@Dhoot you can use the artifact generated during build.

@Dhoot
Copy link

Dhoot commented Apr 16, 2024

Thanks @sidharthv96, but I am looking for some pattern to access using npm so that we can do quick updates in our project and stay on latest version always.

@sidharthv96
Copy link
Member Author

If you're using npm, you can use normal mermaid itself. It'll tree shake and lazy load, without affecting size that much.

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.

Release a mermaid-lite version?
6 participants