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.

Sign up for GitHub

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

Jump to bottom

Display reasons why module was included #85

Closed
wants to merge 1 commit into from
Closed

Display reasons why module was included #85

wants to merge 1 commit into from

Conversation

dgieselaar
Copy link

@dgieselaar dgieselaar commented Jun 4, 2017 •
edited
Loading

First of all, thanks for a wonderful tool! It has been super helpful in our attempts to reduce the bundle size of our main application.

One of the things that was slightly difficult for me though is figuring out why stuff was in the bundle. I started using the reasons metadata in stats.json to trace back the origins, but it's quite cumbersome to grep a JSON file. I thought it would be helpful to have this in the UI as well.

Here's what the change does:

Search for modules.
The user is a presented with an input field in the sidebar, which will allow them to filter a list of modules in the bundle.

Display the reasons a module was included
For every module, the reasons why it was included in the displayed in the module info block. The weird CSS you see in ModuleButton is used to fake a text-overflow from the start rather than the end, because the latter part of the module's path is more identifying than the first few folders.

Adds the concept of selected modules.
This allows the user to select or pin a module, and have its info displayed in the sidebar. There are three ways to select a module:

  • Single-clicking a module in the treemap (note: this changes current behaviour, where a single click triggers zooming in on that module - that is now possible by a double click).
  • Entering a search query in the input field, and clicking one of the modules that are listed.
  • Clicking on a reason of the currently displayed module.

Once a module is selected, window.location.hash is updated with the id of the module. If the user has selected a module group rather than a module, the name of the module is prefilled in the input field to allow the user to search for modules in that group.

The user can deselect a module by clicking on the close button of the sidebar, or they can use a back button that is displayed to go the previously selected module or query. They can also just use browser navigation to quickly navigate back and forth between selected modules. This allows the user to quickly do drill-downs of reasons why modules were included.

image

@dgieselaar dgieselaar force-pushed the reasons branch 2 times, most recently from bfb13db to 70df018 Compare June 4, 2017 12:31
@valscion
Copy link
Member

valscion commented Jun 7, 2017 •
edited
Loading

Hi! Thanks for submitting this PR 😊. Sorry for replying a bit late — I've been thinking about this PR for quite some time and figuring out a good response.

First of all, I really like this feature. I think it brings a lot of additional value that was previously quite inaccessible.

The size of this PR is quite heavy, though, so reviewing this code would take quite a big amount of time. There are a few factors why I haven't been able to review this properly as of yet:

  • All of the changes are in one commit — this means I have to check the entire diff at once instead of small chunks of implementation in steps
  • We don't have any UI tests — seeing as this brings in a major new UI interaction pattern, I fear the worst scenario where this creates much more maintenance load than other features we support.
  • This changes the existing interaction behavior, and I haven't been the one implementing the old behavior so I'm a bit hesitant in making decisions if this new behavior is what we want.
  • The sizes of some component files are getting so large that it's getting increasingly hard to get a full picture of all the things that are happening in those components.
  • This feature brings in a secondary, global state container — the window.location.hash.
  • I'm not sure how @th0r wants us to progress here, or what he thinks. I'm not sure what kind of decisions I'm free to make / how much does @th0r trust me in reviewing and maintaining this package.

All of these make it a bit more challenging to jump into this PR.

However, I really do think that this feature would be a good addition. I hope I'm able to shed some light into why this is a bit difficult PR to review. Do you have any suggestions into how we could make this a bit simpler for me to look through?

@dgieselaar
Copy link
Author

Thanks for replying!

All of the changes are in one commit — this means I have to check the entire diff at once instead of small chunks of implementation in steps

No worries, I can split them up I think. Always a bit of a guess whether commits should be squashed or not.

We don't have any UI tests — seeing as this brings in a major new UI interaction pattern, I fear the worst scenario where this creates much more maintenance load than other features we support.

Understood. If it's necessary I can add some UI tests but it will greatly increase the surface of the PR :).

The sizes of some component files are getting so large that it's getting increasingly hard to get a full picture of all the things that are happening in those components.

I think you're referring to ModulesTreemap? That is getting quite big indeed. I can refactor, but same issue here as well - increases the surface of the PR.

This feature brings in a secondary, global state container — the window.location.hash.

Could you elaborate a bit on why you think this is an issue?

This changes the existing interaction behavior, and I haven't been the one implementing the old behavior so I'm a bit hesitant in making decisions if this new behavior is what we want.
I'm not sure how @th0r wants us to progress here, or what he thinks. I'm not sure what kind of decisions I'm free to make / how much does @th0r truAll of these make it a bit more challenging to jump into this PR.
However, I really do think that this feature would be a good addition. I hope I'm able to shed some light into why this is a bit difficult PR to review. Do you have any suggestions into how we could make this a bit simpler for me to look through?st me in reviewing and maintaining this package.

As far as the last question, I can split it up into smaller commits, and provide a technical overview of the PR (I've basically only described the feature now). I think refactoring and/or adding tests will only make it more difficult to get reviewed/merged. The UI code is still quite simple.

Before I put it in the effort it's probably a good idea to settle on whether this is the way forward. It's been extremely helpful for me the last few days in figuring out why things end up in our bundle.

Maybe we can collect some more feedback before making a decision. I can do a write-up on Medium and ask people to test-drive it.

@valscion
Copy link
Member

valscion commented Jun 7, 2017

This feature brings in a secondary, global state container — the window.location.hash.

Could you elaborate a bit on why you think this is an issue?

It just makes it a bit more difficult to review and test the implications of this and assert that I have figured out the data flow correctly for it. Compared to just looking at this.state and this.props, this is just a bit more to take in at once.

I definitely don't mean that it's a bad thing that this feature leverages window.location.hash. It just adds a dash more complexity :)

@dgieselaar
Copy link
Author

dgieselaar commented Jun 7, 2017 via email

@valscion
Copy link
Member

valscion commented Jun 7, 2017

All of the changes are in one commit — this means I have to check the entire diff at once instead of small chunks of implementation in steps

No worries, I can split them up I think. Always a bit of a guess whether commits should be squashed or not.

Yeah, I don't know about other people but I find it easier to read large-ish PRs with one step at a time, and should I want to squash things, I could do it at merge-time :)

Before I put it in the effort it's probably a good idea to settle on whether this is the way forward. It's been extremely helpful for me the last few days in figuring out why things end up in our bundle.

Maybe we can collect some more feedback before making a decision. I can do a write-up on Medium and ask people to test-drive it.

I'd be curious to hear what @th0r feels about this feature. I do think that this is a very good addition to the plugin. I think it's just about maintenance burden on this one, not necessarily whether people wouldn't consider this a good addition.

Would saving the hash to state on hashchange (setState({ hash: window.location.hash }), and inferring the selectedModule in render(),
rather than imperatively updating state with selectedModule, help?

That could definitely improve things. If we're able to move global hash accessing to the edges of the components and use state & props, it would be great.

I wouldn't even mind if we'd have a separate wrapper component in between that would only handle the integration with window.location.hash and pass some useful value down to other components. This way that component would only have to care about one thing, and we'd have one less thing to worry about in the ModulesTreemap component.

We don't have any UI tests — seeing as this brings in a major new UI interaction pattern, I fear the worst scenario where this creates much more maintenance load than other features we support.

Understood. If it's necessary I can add some UI tests but it will greatly increase the surface of the PR :).

Yeah, that is a tough place to be in. Do you think we could iterate on a good testing strategy outside of this PR? For example, creating a new PR that starts from this branch so we would be able to look at figuring out a good test strategy outside of this new feature addition?

@th0r
Copy link
Collaborator

th0r commented Jun 7, 2017

Hi guys!

@dgieselaar Thanks for a great PR! Information about reasons is definitely the next big thing I want to add to this module and I even have some working code already. The main problem here is the clean intuitive and convenient UI.

Here are my thoughts on UI requirements:

  1. Some interaction with module in a treemap should show it's details including reasons list.
  2. Reasons list should allow to show reason module in a treemap (focus on it): move viewport to it, highlight and zoom if needed.
  3. Reasons list should allow to dig into reason module (and move back to parent module?)

And here are possible solutions:

  1. Cmd+Click on module in a treemap activates it: focuses treemap on it and opens right sidebar with module details. Thus, all the information about bundles and treemap will be located in the left sidebar and all the info about concrete active module will be located in the right one. It will also allow to add any additional module information in future.
  2. Hovering a reason in the list will focus it in the treemap.
  3. Clicking a reason in the list will activate its module.

The blind spot here is moving back to original active module, but I think it can be solved with @dgieselaar's approach that uses browser history.

What do you think guys?

@dgieselaar
Copy link
Author

Hi @th0r! I think these are the only differences between your list of requirements and my implementation:

  • Selecting a module via the tree map is just click rather than cmd/ctrl + click. I actually think it makes more sense to select on click, because of the added value of selecting a module (which IMO is bigger than zooming in).
  • Selecting a module doesn't zoom in. I'm not really sold on the benefits of zooming in automatically, but I don't really mind changing it to zoom in either.

Other than that, the drilldown, and navigating back and forth etc, should work. Have you tried it out?

@th0r
Copy link
Collaborator

th0r commented Jun 7, 2017 •
edited
Loading

Selecting a module via the tree map is just click rather than cmd/ctrl + click. I actually think it makes more sense to select on click, because of the added value of selecting a module (which IMO is bigger than zooming in).

Not sure here. IMO zooming is a quite frequent operation (for me at least) especially in a large project. And I don't think showing module details are going to be used more frequently. And the second concern is we will change default behavior.

P.S. Double-click to zoom doesn't work for me in your branch (Macbook, touchpad).

Selecting a module doesn't zoom in. I'm not really sold on the benefits of zooming in automatically, but I don't really mind changing it to zoom in either.

I'm not sure we need to zoom in but moving into viewport and highlighting is a must-have.

Other than that, the drilldown, and navigating back and forth etc, should work. Have you tried it out?

Yes, I tried but didn't get what moving back and forth does. Seems line nothing changes in the UI.

I think these are the only differences between your list of requirements and my implementation

Also you use the same left sidebar. I was talking about adding the right one to show details about active module.

@dgieselaar
Copy link
Author

dgieselaar commented Jun 8, 2017 •
edited
Loading

Not sure here. IMO zooming is a quite frequent operation (for me at least) especially in a large project. And I don't think showing module details are going to be used more frequently. And the second concern is we will change default behavior.

Alright. How about zooming in on a selected module? A compromise :). If not acceptable, how would you prefer the user to select a module, double click?

P.S. Double-click to zoom doesn't work for me in your branch (Macbook, touchpad).

Will investigate.

I'm not sure we need to zoom in but moving into viewport and highlighting is a must-have.

When you say highlight, do you mean the same behaviour as on hover, namely the module lighting up a bit?

Yes, I tried but didn't get what moving back and forth does. Seems line nothing changes in the UI.

When you say moving back and forth, do you mean navigating back and forth between URLs? If so, it should select the module based on the hash in the URL. Is this not happening for you?

Also you use the same left sidebar. I was talking about adding the right one to show details about active module.

Fair point! Do you think that it's better to have separate sidebars? Personally, I develop in split-screen mode, so my browser window is more tablet size, and having two sidebars take up a lot of horizontal space would make things quite hard to use.

@th0r
Copy link
Collaborator

th0r commented Jun 8, 2017

Alright. How about zooming in on a selected module? A compromise :). If not acceptable, how would you prefer the user to select a module, double click?

Double click (but seems like it has some issues) or cmd/ctrl + click

When you say highlight, do you mean the same behaviour as on hover, namely the module lighting up a bit?

I would change outline color to something noticeable.

When you say moving back and forth, do you mean navigating back and forth between URLs? If so, it should select the module based on the hash in the URL. Is this not happening for you?

It changes URL in the browser, the value in the search and found modules. But how it helps to figure out module reasons?

Fair point! Do you think that it's better to have separate sidebars? Personally, I develop in split-screen mode, so my browser window is more tablet size, and having two sidebars take up a lot of horizontal space would make things quite hard to use.

Actually, when you selecting a module there is no point in showing left sidebar - only right sidebar will be visible with details module information in it.

@dgieselaar
Copy link
Author

It changes URL in the browser, the value in the search and found modules. But how it helps to figure out module reasons?

The module reasons are displayed below the selected module. Might be a bug somewhere, could you make a screenshot?

@th0r
Copy link
Collaborator

th0r commented Jun 8, 2017

1

@dgieselaar
Copy link
Author

Haha, well, that explains, a bug indeed. Seems like the names don't correspond. Could you send me the stats file?

@th0r
Copy link
Collaborator

th0r commented Jun 8, 2017

https://www.dropbox.com/s/qdm8abmu179pnom/stats.json?dl=0

@dgieselaar
Copy link
Author

dgieselaar commented Jun 8, 2017 via email

@dgieselaar
Copy link
Author

Sorry, completely forgot about this! Moving it back to the top of my list :).

@dgieselaar
Copy link
Author

Hi @th0r, I made the following changes:

  • The bug which caused your modules not to be selected is fixed.
  • I've reverted the click-to-select change. The behaviour is now as it was: single select zooms in on a module. If event.ctrlKey or event.metaKey is true, it selects the module instead of zooming in.

I've not yet created a second sidebar. Can you try this out first and tell me what changes you'd like to see based on this?

@valscion
Copy link
Member

valscion commented Jul 7, 2017

Hi! Thanks for continuing work on this PR :) I'd like you to know that we haven't forgotten about this PR. It's just that this has lots of new changes, and we don't have any proper UI tests, that adding new features to the UI is starting to be much more scary maintainability-wise.

Would you happen to have any ideas on how we could possibly add even rudimentary UI tests for this?

@mshustov
Copy link

hi, guys, I did something similar https://github.com/restrry/webpack-deps-tree what do you think about integrating that tool in webpack-bundle-analyzer ?

@valscion
Copy link
Member

hi, guys, I did something similar https://github.com/restrry/webpack-deps-tree what do you think about integrating that tool in webpack-bundle-analyzer ?

Once we have #32 done, that visualization could be a new reporter that you could develop outside of this repo :)

But let's not hijack this PR to discuss this, though

@valscion valscion mentioned this pull request Jul 26, 2017
Closed
@valscion
Copy link
Member

valscion commented Feb 4, 2018

Hi @dgieselaar, sorry for dragging this PR out for so long! The idea behind this PR is amazing, and would definitely be useful to have in some form.

However, with the upcoming version 3, I'm afraid we can't continue with this PR as it currently is. Hopefully in the future it will be simpler to experiment with new analysis tools and extra information. Right now, I'm concerned of the merge conflicts we'd have should we try to get this in before version 3.

I'll close this PR as there doesn't currently seem to be a way to get this in. Thank you for contributing, it has been a pleasure talking this idea through and I hope we'll see some version of this idea later in the future 💞

@valscion valscion closed this Feb 4, 2018
@dgieselaar
Copy link
Author

dgieselaar commented Feb 4, 2018 via email

@valscion
Copy link
Member

valscion commented Feb 4, 2018

Let's see how the code landscape looks like when I get v3 finished up. It might be a good idea to wait for v3 first before opening up this code again ☺️

@swrobel
Copy link

swrobel commented Sep 25, 2018

FYI @dgieselaar, v3 has been released!

@dgieselaar
Copy link
Author

@swrobel yeah, I think @th0r will pick it up IIRC!

@valscion
Copy link
Member

The version 3 I talked about 8 months ago didn't go so well and I had to scrap my PR. The other version 3 that was released is something else

@th0r
Copy link
Collaborator

th0r commented Sep 26, 2018 •
edited
Loading

Displaying import reasons is what I'm working on right now so expect it to be landed in the next version.

@th0r th0r mentioned this pull request Apr 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
type: Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dgieselaar @valscion @th0r @mshustov @swrobel