-
-
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
Split chunks into individual dirs #4733
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## sidv/esbuildV11 #4733 +/- ##
================================================
Coverage 46.09% 46.09%
================================================
Files 53 53
Lines 6736 6736
Branches 32 32
================================================
Hits 3105 3105
Misses 3630 3630
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. |
@aloisklink, what's you opinion on shipping all the They are the main factor to this size increase. The dist folder is only 14.7 MB without the map files. I just realised that the source maps were not linked from our files, so they were essentially not being used by the browser. So, I think we can remove the maps entirely in v11, and add it back if there are any complaints?
Without mapspackage size: 4.4 MB With mapspackage size: 16.0 MB Savings/week with 4,55,756 downloads.Bandwidth: ~50TB |
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.
@aloisklink, what's you opinion on shipping all the .map files?
It will significantly reduce the npm install bundle size. Which all .map files are actually needed? (Does core need maps?)
I'd love to have map files, but I think they're currently broken. All of the map files point to the ./src/
dir, but we don't upload the ./src/
dir to NPM, so I don't think they do anything currently.
This is a shame, map files would have helped a lot when I've previously been trying to debug bugs in Mermaid.
My feeling is that:
- map files are nice to have, but are probably only required by in the
.core.esm.mjs
build, since that's the build that NPM/JavaScript devs are most likely to use. - map files need the
src/
dir, but we can't really easily add that to the NPM dir, because thesrc/docs
folder has a lot of junk in it (e.g. Vitepressnode_modules
folder??)
So maybe remove the map files for now, and once move the docs out of the packages/mermaid
folder (see #4382), we can consider adding the map files for core.esm.mjs
back in?
As another file saving, we could make both mermaid.js
and mermaid.min.js
the same file (same with mermaid.esm.mjs
and mermaid.core.esm.mjs
). I know we need to keep both files for backwards compatibility, but I feel like there's no reason why both files can't be the exact same minimized version.
I just realised both are actually the same. But shouldn't mermaid.js be the non minified version? |
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.
But shouldn't
mermaid.js
be the non minified version?
It currently should be the non-minified version, yes. But why do we even need to publish a non-minified version? IMO, the only benefit is the non-minified version is easier to debug.
Although, that's probably something to change in a future PR! I'll say keep the old behavior (mermaid.js
and mermaid.esm.mjs
are non-minified) now. We can then make another PR in the future (maybe even for a Mermaid v11.1 release), that will minify them to save space. That PR will probably need to get a 👍 from more reviewers, since although I think it should be a non-breaking change, touching the entry-points is always high risk.
📑 Summary
This will help downstream users if they want to include only a subset of the dist in their bundles.
The files below do not have chunks, hence the missing folders.
Resolves #4203