-
Notifications
You must be signed in to change notification settings - Fork 4
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 HTML reporter #7
Conversation
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.
Looks good, really appreciate you putting this together.
const toSizes = modules => { | ||
const sizes = {} | ||
for (const { id: idRaw, size } of modules) { | ||
const id = idRaw.replace(/^\0(?:commonjs-proxy:)?/, '') |
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.
I'm somewhat torn on including this as is. I think it is logical to remove that for most users. My only hesitation is, since that id is attributable to a plugin and not Rollup core should it be built in? While unlikely, it has slippery slope potential.
Another potential argument is that particular prefix should stay to alert users that the module in question likely isn't benefiting from tree-shaking.
All that to say, everything else looks good so I'm going to merge it then add an option for modifying ids. That will apply to both the text output as well as the chart.
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.
Yeah, I'm not crazy about it either. The \0 prefix seems like a pretty standard thing in Rollup land, so that could be separate from the CommonJS plugin prefix. I just hardcoded it because the plugin is so common.
@@ -48,5 +48,8 @@ | |||
"rollup55": "npm:rollup@0.55.x", | |||
"rollup60": "npm:rollup@0.60.x" | |||
}, | |||
"dependencies": {} | |||
"dependencies": { |
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.
Happy to see you were able to implement it with minimal new dependencies introduced.
Thank you so much for taking this on. It's a great addition. Will be interested to see if you do find the other method showing deps to be a better overall approach. As previously mentioned I'm rather indifferent myself on charts so I don't have much to add on either approach. I'm going to keep an eye on the 1.0.0 release of Rollup. If it appears to be imminent I'll likely hold off until it release before doing the semver major bump, to ensure there aren't any other significant API changes that I need to make to accommodate. I'm also planning to drop support for versions before 0.60.0, as the hook I'm using to support those is deprecated in 1.0.0. If it seems 1.0.0 is still a ways off I'll go ahead and bump the package to include the charts and then release the named export changes with 1.0.0. |
I might play around with other visualizations altogether. Perhaps I can get GraphViz to produce an illustrative graph where node sizes represent module sizes. It's definitely an interesting challenge to visualize this stuff in a meaningful way. From what I've read, the Rollup team are progressively changing the APIs to accommodate the 1.0.0 release, but indeed, that might take a while. Looking forward to the next version, although it will really just allow me to move my own code from my package to yours at this point. :-) |
This adds the ability to create a report in the form of a sunburst chart, as discussed under #5.
This initial version flattens the dependency graph. While that gives you a nice overview of which packages contribute most to the bundle size, it does not show which package introduced which dependencies.
I've been experimenting with the latter on my
html-report
branch. That one creates a far more elaborate sunburst chart, where shared dependencies are repeated for everything that depends on them. Because this breaks total sizes, I'm not convinced about the approach just yet.