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

Sunburst chart #25

Closed
wants to merge 7 commits into from
Closed

Sunburst chart #25

wants to merge 7 commits into from

Conversation

valscion
Copy link
Member

@valscion valscion commented Nov 27, 2016

Hi there!

I wanted to see if I could get a sunburst chart out of my dependencies, and after hacking on this for a while I've got a proof of concept working.

sunburst-charts

I drew my inspiration from the great work by @chrisbateman in https://github.com/chrisbateman/webpack-visualizer and I owe him for the amazing tool that webpack-visualizer is.

Note that this is still a proof of concept. Should you be interested in having this feature available built-in for webpack-bundle-analyzer, this will require some tweaks:

  • Right now, sunburst chart sizes are calculated only based on statSize
  • I'm not sure this would work with the static build
  • The sunburst chart is very barebones
  • The colors don't seem to be meaningful
  • Super-tiny modules should probably be filtered out from this (like webpack-visualizer does)

What do you think? Would this be something you'd be interested to accept if I'd keep on working on this?

The Yarn lockfile changes were created by v0.17.9 of `yarn` after adding
the `d3` package as a dependency with `yarn add d3`
The lockfile changes were received by yarn version 0.17.9
@valscion
Copy link
Member Author

valscion commented Dec 6, 2016

Hiya! I know this PR is quite a big change compared to what there was before. Would you be interested if I'd look into some sort of plugin system that would work with this code base easily? Or should I rather create a sibling project with this sunburst functionality?

I'd be happy to try to reduce the footprint this feature has in the codebase and help my best with tackling issues possibly raised in the future because of this. And if I'd ever become disengaged and no-one else wants to step up for maintaining this feature, I would aim for this to be easily removed.

I value any comment about this, even if it's a rejection to this whole idea 😄. I'd be happy to chat about the future of webpack-bundle-analyzer, as bundle sizes and comprehensible visualizations about performance problems are close to my heart.

@th0r
Copy link
Collaborator

th0r commented Dec 7, 2016

@valscion Thanks for PR!

As you mentioned there is an amaizing webpack-visualizer already and I wouldn't make my own module, but there are a couple of things that made me create it:

  1. It's easier for me to read treemap chart than sunbirst one: IMO it's a little more human-friendly.
  2. I missed information about module sizes after minification.

Adding second type of chart to this analyzer will significantly increase it's complexity adding IMHO rather questionable benefits.

Why wouldn't you just use webpack-visualizer if you like sunburst charts more?

@valscion
Copy link
Member Author

valscion commented Dec 7, 2016

Thanks for replying!

It's easier for me to read treemap chart than sunbirst one: IMO it's a little more human-friendly.

I think so too, treemaps are faster and easier to read. Sunburst charts that zoom are easier for me to browse, though, if I want to dig deeper into some large-ish modules.

I missed information about module sizes after minification.

That is also the reason why I don't use webpack-visualizer. It's architecture and the way it calculates the sizes does not allow me to see the post-minify and post-gzip sizes, which is why I think this project is superior to it.

Why wouldn't you just use webpack-visualizer if you like sunburst charts more?

I don't actually think that this is a choice between treemaps or sunburst charts. I like treemaps for seeing the larger picture, but sometimes I'd like to see a nice sunburst chart to allow for different kind of visualization.

I figured that sunburst charts could help me discover something that cannot be as easily seen from a treemap. I also think that sunbrust charts are easier to browse and dig deeper than treemaps, as e.g. sunburst charts don't require you to scroll your window but it keeps its position and size.

Adding second type of chart to this analyzer will significantly increase it's complexity adding IMHO quite questionable benefits.

Yeah, I'm not sure either whether this is worth the increased complexity. That is why I have been thinking whether there could be another approach to make this work.

What I'd need for sunburst charts would be the same stats output that this plugin provides on the webpack side. I was wondering whether the webpack plugin could be modified somehow so that its output could be used in a totally different UI as well? Or that the UI that is opened could be configurable, so that I could build a totally separate npm package that would only contain the UI parts that could be plugged into this packages webpack parts.

I haven't yet formed any concrete plans, but I can think about what this "separation of webpack plugin and UI" would mean code-wise if you think that there could be some benefits to reap. It might even make the current code easier to reason about, as there probably would be an even more clear distinction between the current UI and the webpack plugin than there is right now. Or it could be that the current situation is the best approach and any changes to it would make the code more difficult to understand 😅

This PR was meant to be just a conversation starter about future directions, and I wanted to see what all the possibilities of this plugin are. I was very excited to learn that the JSON output of bundles is so powerful in this plugin that there is likely multitude of different things that JSON could be used for, sunburst chart being only one of them.

If we could make the JSON data more easily consumable and pluggable, people could come up with wild ideas from CLI commands to very app-specific visualizations. I think that this project has a big potential in allowing such experiments 😄

@valscion
Copy link
Member Author

valscion commented Dec 7, 2016

And funnily enough, chrisbateman/webpack-visualizer#13 even says that this project has access to more info and should probably be used instead 😄

@th0r
Copy link
Collaborator

th0r commented Dec 7, 2016

Or that the UI that is opened could be configurable, so that I could build a totally separate npm package that would only contain the UI parts that could be plugged into this packages webpack parts.

That's a very interesting idea and, at first glance, it won't be too hard to split analyzer from report UI.

We can move all current UI code from webpack-bundle-analyzer into, say, webpack-bundle-analyzer-treemap-reporter.
webpack-bundle-analyzer will be responsible only for analyzing bundle and feeding this info to reporter-plugins whose responsibility will be showing it in any way.

And the nice thing is reporter is not limited to some UI stuff - it can do anything it want with provided data!

@valscion Does it makes sense?

@valscion
Copy link
Member Author

valscion commented Dec 7, 2016

Yes! That is exactly what I envisioned the best solution could look like!

I might have some free time during the christmas holidays to look into this, so unless your fingers are itching to take a stab at this, I'll happily devote some time to dig deeper into this library and come up with some sort of an action plan to tackle the separation.

I'm also fine by it if you want to do it all by yourself, but I suppose more happy code monkeys helping with open source isn't a bad thing? 😄 🐒

I think it might make more sense to start a new bigger issue for this whole separation thing to keep track of the process, and to provide an easier place for discussing related changes to this. We can close this PR in the mean time, I'll keep my branch around so that I can refit this visualization for the new architecture.

@th0r
Copy link
Collaborator

th0r commented Dec 7, 2016

I might have some free time during the christmas holidays to look into this, so unless your fingers are itching to take a stab at this, I'll happily devote some time to dig deeper into this library and come up with some sort of an action plan to tackle the separation.

Great! Go ahead, man! 😉

I think it might make more sense to start a new bigger issue for this whole separation thing to keep track of the process, and to provide an easier place for discussing related changes to this.

Agree. Could you create it?

@bitpshr
Copy link

bitpshr commented Dec 7, 2016

Related, I added sunburst support as well (I didn't know @valscion's PR existed).

@th0r, you built a great tool and we plan to use it as a basis for Dojo 2 build reporting. We were concerned about shipping the Carrot Search commercial treemap solution to users with permanent demo branding, hence our fork.

sunburst

@th0r
Copy link
Collaborator

th0r commented Dec 7, 2016

@bitpshr Nice! So, after splitting analyzer and reporter you'll be able to extract it into separate reporter package and just plug it to analyzer.

@valscion
Copy link
Member Author

valscion commented Dec 8, 2016

Wow, @bitpshr that is cool! And much prettier than what I had hacked together, but oh well, mine was just a proof of concept and a conversation starter 😄

Great to see there's more interest for #32! I'd value your input once I get something going on regarding the splitting, so make sure to subscribe to that issue if you'd like your voice to be heard 😉

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.

3 participants