-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Use ESBuild (replaces UMD with IIFE bundle) #4729
Conversation
We could release the mermaid-mini bundle without elk and cytoscape. The size difference is significant. Simply adding the dependencies to external does the job. We need a way to inform the user why the diagram renders fail though. Even better, if we could dynamically avoid bundling the diagrams, that would save more space. @Yokozuna59 this would help you with vscode dependencies too. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #4729 +/- ##
===========================================
- Coverage 71.57% 46.09% -25.49%
===========================================
Files 116 53 -63
Lines 12709 6736 -5973
Branches 549 32 -517
===========================================
- Hits 9097 3105 -5992
- Misses 3495 3630 +135
+ Partials 117 1 -116
Flags with carried forward coverage won't be shown. Click here to find out more. |
@aloisklink changing vite's umd -> iife did work for mermaid, but had issues for other packages. So, it'll need more changes, which is more work that will be removed in the next commit. So, I have squashed all the commits from the old branch to this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good to me!
Simply adding the dependencies to external does the job.
IMO, I think it would be better to completely remove the mindmap
and flowchart-elk
diagrams instead of doing this. But we'd first have to split them into separate @mermaid-js/mermaid-mindmap
and @mermaid-js/flowchart-elk
NPM packages, which would take quite a bit of work, see #4120
Todo before merging
- Can you change the
.vite/jsonSchemaPlugin.ts
path to.build/jsonSchema.ts
in:# - `.vite/jsonSchemaPlugin.ts` # you may need to add it to `.vite/jsonSchemaPlugin.ts` and `src/docs.mts`
- How are we going to document the BREAKING CHANGE that the UMD
dist/mermaid.min.js
is now an IIFEdist/mermaid.min.js
file?- This must be in the PR description (I'd even recommend sticking this in the commit description if you can, if you're comfortable with
git rebase
) - Maybe we also need a label for PRs to that says BREAKING CHANGES?
- Can we stick this in the PR title too? Something like
Use ESBuild (replaces UMD with IIFE bundle)
- This must be in the PR description (I'd even recommend sticking this in the commit description if you can, if you're comfortable with
- When to merge: Do we have a rough timeline on when we'd release v11? If it's going to be a long way in the future, I'd recommend holding back on merging this until closer to v11, just because it's always easier to fix merge conflicts in PRs, rather than in the
main
/develop
/next
branches.
Also, this is a pretty minor nit-pick, but generally a change like this won't be a feat:
, but instead it would be a build:
or chore:
, since it's not a feature to users of Mermaid. It's mainly just for mermaid developers.
The define feature could be used to avoid the import of these without breaking them to separate modules. We could use define to replace that import with a custom error diagram import, which mentions they are using a smaller version of mermaid and some diagrams are not available in that. |
mermaid.min.js and mermaid.js will now be IIFE instead of UMD.
Co-authored-by: Alois Klink <alois@aloisklink.com>
Co-authored-by: Alois Klink <alois@aloisklink.com>
24e1048
to
8f44de6
Compare
This, Langium and your type change are the only changes that we have planned for v11. |
Top of the changelog like V10 is mandatory.
Done
Aren't all PRs to
Done |
Sounds good!
A PR to Also, if we make a BREAKING CHANGES label, we can stick it into the The Mermaid project doesn't have great Release Notes, (even I sometimes struggle to realize what's changed when I'm maintaining the |
Hmmm, I don't think we need the Minor/Patch labels, since I feel like Type: Enhancement/feature and Type: Bug/Error/fix should already cover these (since a Semver minor change is something that we added, and a semver patch change is just a fix). Maybe it's worth re-opening #4444, even if this is more to do with PRs than issues. |
Done :) |
📑 Summary
Replace vite with esbuild for builds.
Vite will be used for coverage builds.
📏 Design Decisions
🚀 speed.
📋 Tasks
Make sure you
develop
branch