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

Version 3: Split reporting UI from dependency graph size calculation #97

Closed
wants to merge 71 commits into from

Conversation

valscion
Copy link
Member

@valscion valscion commented Jul 14, 2017

This is a tracking PR for getting to version 3.

What? What is version 3?

Version 3 will split the reporting UI (the treemap reporter) from the webpack code.

This allows users to create any kind of reporter, while the webpack plugin and CLI would provide the raw data containing post-minification and gzipped sizes.

How can you help?

Please leave me a comment on your thoughts on this idea, or even add a reaction to this PR description so I could see how many are interested in this at all.

How did I come up with this idea?

The thing that started this eventual process was an experimental PR #25 — I wanted to see if I could add a sunburst chart visualizer to webpack-bundle-analyzer alongside the treemap visualizer.

It seems that I wasn't the only one, @bitpshr had also done a much more finished version of a sunburst chart:

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

And sunbursts are just one possible visualizer. What about a VR-based bundle content visualizer? https://twitter.com/ken_wheeler/status/872939627656728578

What has happened before this PR?

I have figured out how webpack-bundle-analyzer works and pondered what would be the best approach to have a custom reporter.

And then me and @th0r figured that the best solution would be to split analyzer from the treemap UI code: #32

I've tested various different approaches to this problem: #40 (with explanation in #32 (comment)), a proof of concept of this in valscion#1 and generally just figuring things out.

Architectural changes

This repository is converted to a monorepo (like Babel), and the maintenance work is handled with Lerna.

New packages (to be) created:

  • webpack plugin code
  • Treemap reporter that uses the Carrot Search Foamtree
  • Bundle content parser
  • Logger
  • Compatibility package to tie these together exactly like existing webpack-bundle-analyzer plugin works to help in migration

These package splits are still not set in stone, and I'll evaluate the best choices for split points as I go.

TODO (that will change as I think things through)

I intend to work on this by creating new PRs for distinct features that point to this branch instead of master. This way I can work in smaller steps, and hopefully have more focused PRs.

  • Merge in a refactoring that moved chart data calculation to source code edges, Move chart data calculation to source code edges #81
  • Proper scripts for the common development cases
  • Run build automatically for all packages before publishing new versions
  • Move all reporter related configuration options under reporter specific options object
  • ~~Improve the current test infrastructure~~~
    • Postponed to a later time
  • Create a new package that has exactly same API as v2 does, but just utilizes these new packages
    • This should make migration cost zero and still allow using a different visualizer by just using a different package as the entry point.

Nice things to have:

  • Introduce Flow, at least to the chartData boundaries, and use the same types in reporter and plugin
  • Maybe add some way of validating reporterOptions which reporters themselves can use

Switch lerna and yarn to use workspaces
This commit is part of the `master` branch, the merge commit of PR #121
This commit is part of the `master` branch, the merge commit of PR #122
This commit is part of the `master` branch, the merge commit of PR #131
@@ -3,6 +3,7 @@ sudo: false
branches:
only:
- master
- version-3
Copy link
Member Author

Choose a reason for hiding this comment

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

This will need to be removed right before merging. This is needed here so that branches against this version-3 branch have their tests ran in CI. See #143 (comment)

@valscion
Copy link
Member Author

I think I might get to this again at some point, but this branch has lived for long enough so far that continuing from here would cause undue hassle. I'll close this PR for now.

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.

None yet

1 participant