webpack-contrib / webpack-bundle-analyzer

Webpack plugin and CLI utility that represents bundle content as convenient interactive zoomable treemap
MIT License
12.57k stars 484 forks source link

feat: gray out non-matches when searching #554

Closed starpit closed 1 year ago

starpit commented 1 year ago

This PR reduces the saturation (from 60 to 10) for non-matches.

Note: there seems to be a bug in foamtree? It does not redraw some of the nodes until you hover over them. This seems to be a preexisting issue.

Fixes #553

Screenshot 2023-01-30 at 11 00 33 AM
Click to see the outdated details about the .redraw() behavior we've discussed in this PR ## FoamTree bug? This gif shows how randomly some of the nodes are not redrawn (by foamtree) until you hover over them. I think this bug is masked by the current highlighting behavior, which does not change the color of non-matching nodes. ![foamtree-oddity](https://user-images.githubusercontent.com/4741620/215529107-076ee052-8913-4013-bf0d-90194bc53fe5.gif)
valscion commented 1 year ago

This gif shows how randomly some of the nodes are not redrawn (by foamtree) until you hover over them. I think this bug is masked by the current highlighting behavior, which does not change the color of non-matching nodes.

Hmm, if this bug wasn't active earlier but is now visible, we will need to fix it before we can ship this feature.

Are you able to debug further on why this happens and if updating our version of FoamTree would help fix the situation?

Ever since https://github.com/webpack-contrib/webpack-bundle-analyzer/pull/412 we've got FoamTree from an NPM package. I don't recall right now on how updating this package will happen so it needs some thought behind it. Looking through the changelogs of FoamTree could show if this bug has existed on FoamTree side and has now been fixed.

starpit commented 1 year ago

Hmm, I don't think FoamTree is open source? I don't even see a way to file an issue. You are using the latest version (3.5.0).

valscion commented 1 year ago

Yeah FoamTree is not open source but they do distribute it via npm

starpit commented 1 year ago

Interesting. If i change the .redraw(false, groupsToRedraw) to .redraw(), then the bug goes away. So either: a) the groupsToRedraw computation is incorrect on your side; or b) foamtree does not handle this properly.

Is it viable to work around this for now by always redrawing all groups, i.e. .redraw()?


Edit: Thinking this through more: if the highlight groups changes (which is where this redraw() is called)... then with this PR, we will always be redrawing almost all of the chunks... since the colors are likely to change for almost all groups, when the search term changes.

I have updated the PR along these lines.

valscion commented 1 year ago

Thanks for looking more into why this case happened 😊. I'm not that familiar with the treemap interaction code so it is very helpful to have you figure this out, too.

It sounds like the original code could've used the false argument as some sort of render optimization. Is the rendering now noticeably slower than it used to be? I wonder if we need to somehow have some sort of a debounce for the handling of search input value changes if redrawing the treemap becomes slow.

starpit commented 1 year ago

the false of .redraw(false) is the default behavior. it tells foamtree not to animate the update. the second argument is a hint to foamtree as to which groups may have changed. the bug is either in bundle analyzer's presentation of changed groups to foamtree, or in foamtree's algorithm that interprets this hint.

i have not noticed any rendering lag for the example above, which has maybe 100 top-level groups, and 600 total groups.

valscion commented 1 year ago

Okay thanks for the extra information!

That sounds like a useful optimization to have. I suspect that operation is costly so with users having even more groups and maybe a less powerful machine could get performance issues they weren't seeing earlier.

However, we are indeed debouncing changes done to the search input here:

https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/38930418af195b5ecbc857ce1db43bf83256d726/client/components/Search.jsx#L42-L44

so maybe the performance issue wouldn't be that bad. I don't know what other code paths call to this method that now calls .redraw()? I hope none related to cursor movements or some other commonly used events.

valscion commented 1 year ago

This is now in v4.8.0! 🚀