Display reasons why module was included

[dgieselaar]

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.

[valscion]

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]

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]

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]

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?

…

On Wed, Jun 7, 2017 at 9:18 AM Vesa Laakso ***@***.***> wrote: 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 :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#85 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVh3LDy_6xFqwcxbpXz6d63SMxg5XF5ks5sBk68gaJpZM4NvTHy> .

[valscion]

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]

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]

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]

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]

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]

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]

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]

[dgieselaar]

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

[th0r]

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

[dgieselaar]

Thanks! Will report back later.

…

On Thu, Jun 8, 2017 at 10:41 AM Yuriy Grunin ***@***.***> wrote: https://www.dropbox.com/s/qdm8abmu179pnom/stats.json?dl=0 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#85 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVh3O4IUxCaXl32DvFJqtBYLCb72gzEks5sB7OegaJpZM4NvTHy> .

[dgieselaar]

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

[dgieselaar]

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]

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]

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]

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]

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 💞

[dgieselaar]

No problem, thanks for notifying me! I hope I can pick it up soon when I have some time available.

…

On Sun, Feb 4, 2018, 11:16 Vesa Laakso ***@***.***> wrote: Hi @dgieselaar <https://github.com/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 💞 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#85 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVh3C0ytcPBjIEggZPinu5imdpU27xAks5tRYN_gaJpZM4NvTHy> .

[valscion]

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]

FYI @dgieselaar, v3 has been released!

[dgieselaar]

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

[valscion]

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]

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