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

Revisit the decision to allow webpack analyzer tools #4563

Closed
gaearon opened this issue Jun 5, 2018 · 47 comments
Closed

Revisit the decision to allow webpack analyzer tools #4563

gaearon opened this issue Jun 5, 2018 · 47 comments
Milestone

Comments

@gaearon
Copy link
Contributor

gaearon commented Jun 5, 2018

I haven't been following #3945 closely and it just got merged. But I'm not so sure this is a great idea (see #1858 (comment) for why).

I'll want to look at this again closer to shipping 2.x and decide whether to ship or remove this.

@audiolion
Copy link

I came looking to answer this question:

Given a PR, what changes were there to the bundle size overall and across various chunks

Very much like facebook/react

The PR providing the stats file will let me do that without ejecting, so I am in favor of it. Would source maps be able to provide this info?

@gaearon
Copy link
Contributor Author

gaearon commented Jun 5, 2018

You can get this info by reading the file sizes from the files, no? I don't think you even need sourcemaps for that.

@valscion
Copy link
Contributor

valscion commented Jun 5, 2018

Yeah, I undersand if #3945 wouldn't in the end be what you'd like CRA to have. It does tie CRA closer to webpack specific tooling than what probably is the goal of CRA in the first place.

I haven't used much sourcemap-specific tooling, due to sourcemaps so easily getting broken and it only takes one bad configuration to break them.

I am biased, as I help maintain webpack-bundle-analyzer, but I have yet to see a tool that works on sourcemaps and allows us to visualize the post-gzip sizes of each chunk and module in a visually pleasing way.

I hope this situation will change in the future, and that we will see better tooling for sourcemap analyzing than what there currently are.

@joshwcomeau
Copy link
Contributor

Hm, so yeah, I understand that the solution isn't ideal, since we want to avoid having webpack-specific tooling (after all, webpack should be an implementation detail and shouldn't affect the surface API). I thought it was agreed that the benefits outweighed that con, though.

I agree with the philosophy in #1858 (comment), but the reality is that the webpack-specific tooling is more helpful/insightful than sourcemap-specific tooling (at least, that's been my experience. If we can provide instructions for how to achieve the same result with the same ease-of-use, that's a different story).

It's discouraging to hear that this may not make it in to 2.0. It may not be evident from the small # of changed lines, but it still took a considerable amount of my time, over many months, during a time that I had a lot of things vying for my attention. I was happy to spend that time and energy, because I think it's an important feature, but I would not have spent it if I had known that the feature may not be released.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 5, 2018

I mean, I appreciate the work. I've literally not even tried this yet, so I want to make sure I play with it before it's shipped. I try to make sure to try all new features we add but I haven't had time to look at this one yet.

If the experience is good and the info itself is representative of the builds (and not misleading like webpack-specific tools commonly present it), I agree it's worth shipping in this form even if we remove it later (e.g. when there are better sourcemap-based tools).

@gaearon
Copy link
Contributor Author

gaearon commented Jun 5, 2018

I haven't used much sourcemap-specific tooling, due to sourcemaps so easily getting broken and it only takes one bad configuration to break them.

Where can I learn more about this?

@bugzpodder
Copy link

bugzpodder commented Jun 5, 2018

Here is my notes on testing yesterday. This should have been done more throughly before merging but I did not realize using this was somewhat controversial at the time. In general I feel like we should not be adding unnecessary confusion to the users over features we add so I'd like ask others to test this as well and weigh in.

What I like over webpack-bundle-analyser over source-map-explorer:

  • It shows you the all the chunk's information (also has a hidden left nav to select files). With source-map-explorer only ever display one chunk at a time. (you can specify main., vendor., or one of many chunks one at a time). Realistically most people only care about the main/vendor bundles so this isn't really a big deal I think?
  • It has more information: stats, gzipped, parsed size over just parsed size for sourcemaps.
  • Having it in one file is slightly easier to deal with if you want to compare before and after.
  • It has slightly better resolution in terms of some assets, for example I have imported moment-timezone and webpack-bundle-analyser more correctly identified a json file that was bundled as belonging to moment-timezone (150kb) while soucemaps just lumped it into another random module!
    Both should be the same json.

screen shot 2018-06-05 at 10 20 57 am

screen shot 2018-06-05 at 10 20 42 am

What are similar:

  • Both tools have similar UI, webpack-bundle-analyser has a hidden left bar to select file while source-map-explorer's tree navigation is a little bit cleaner.
  • Both reports very similar +/- a few kb module parsed size and total parsed size in most respects (except for the exceptions I mentioned)

What I am concerned about webpack-bundle-analyser:

screen shot 2018-06-05 at 10 14 22 am

screen shot 2018-06-05 at 10 14 10 am

screen shot 2018-06-05 at 10 13 57 am

@gaearon
Copy link
Contributor Author

gaearon commented Jun 5, 2018

It only takes account of the bundle size before uglify kicks in? This is somewhat well doumented: webpack-contrib/webpack-bundle-analyzer#161 https://twitter.com/dan_abramov/status/866424533787586560?lang=en

This seems worrying if it's the case. I assumed that this has long been fixed given that folks kept pushing for that PR to get merged.

Indeed, we can't possibly ship this if chrisbateman/webpack-visualizer#31 is still an issue. This is a React project, and we'd be misreporting the size of React itself. This doesn't make sense. :-)

@gaearon
Copy link
Contributor Author

gaearon commented Jun 5, 2018

Oh wait, https://github.com/chrisbateman/webpack-visualizer is a completely different project. So maybe it's not a problem for webpack-bundle-analyzer after all?

@audiolion
Copy link

You can get this info by reading the file sizes from the files, no? I don't think you even need sourcemaps for that.

Hey yeah absolutely, but getting gzipped sizes of webpack tree-shaken chunks and such? Not so simple without that stats file

@gaearon
Copy link
Contributor Author

gaearon commented Jun 5, 2018

@audiolion I'm not sure I understand. The chunks are just JS files. You can read every file in static/js—those are your chunks. Then you can gzip them. In fact that's exactly what we do in our build scripts to show the size change on build:

var fileContents = fs.readFileSync(path.join(root, asset.name));
var size = gzipSize(fileContents);

@audiolion
Copy link

audiolion commented Jun 5, 2018

thanks for the reply, my mistake, I thought that just gzipping it wouldn't give the same results as webpack's uglify + gzip, but of course the build is already minified.

on a fun side-note, that reporter file is utilizing the webpack stats file :)

var assets = (webpackStats.stats || [webpackStats])

@gaearon
Copy link
Contributor Author

gaearon commented Jun 5, 2018

@audiolion That's fair but we also write equivalent info into asset-manifest.json so you should be able to use that.

@audiolion
Copy link

audiolion commented Jun 5, 2018

yes absolutely, thanks for your help! I was able to track size + gzip sizes of my files during our CI process with current react-scripts@next behavior (not using #3945)

@joshwcomeau
Copy link
Contributor

I mean, I appreciate the work. I've literally not even tried this yet, so I want to make sure I play with it before it's shipped. I try to make sure to try all new features we add but I haven't had time to look at this one yet.

If the experience is good and the info itself is representative of the builds (and not misleading like webpack-specific tools commonly present it), I agree it's worth shipping in this form even if we remove it later (e.g. when there are better sourcemap-based tools).

Awesome, thanks for the clarification :) I misunderstood the original comment as "I'm not sure we should ship this on principle", but of course if the feature is problematic/misrepresentative, that's totally different. If that is the case, it'd be worth trying to raise signal on that somehow; even if we don't provide a first-class option for it, I believe this is how most devs are analyzing their bundles, post-eject. And so if this is producing misleading results, that seems like it's already a big problem worth addressing.

And yeah, I also agree that the best-case scenario is that we replace this work with a generic solution with the same benefits, I'd be happy to see that happen.

@bugzpodder thanks for testing this - I wasn't aware of the potential issues with it being done pre-uglify. That seems like it would have massive implications, given that uglification removes dead-code branches :/

@valscion
Copy link
Contributor

valscion commented Jun 6, 2018

I would like to clear the misconception that webpack-bundle-analyzer would operate before uglification. That is not the case.

webpack-bundle-analyzer operates by parsing the output bundles, ones that have already been minified by uglify, and then mapping webpack generated module IDs inside those bundles back to the originating module name by looking them up from the stats JSON file.

The sizes webpack-bundle-analyzer shows come in three forms, where "parsed" is the exact size as it is in the output file, "gzip" is the sizes of those same modules after running them through gzip and finally "stats" is the original size of the module.

In that sense, webpack-bundle-analyzer should not have the same issues with showing inaccurate bundle sizes as some similar webpack tools have, like webpack-visualizer or webpack-bundle-size-analyzer mentioned before by referring to Dan's tweet.

I'll answer the rest of questions in more comments to come ☺️

@bugzpodder
Copy link

bugzpodder commented Jun 6, 2018

Is webpack-contrib/webpack-bundle-analyzer#161 a non-issue in this context then? I was testing this in CRA and was only able to get some discrepency in a very limited respect (ie under the exact same conditions from that issue).

I am having trouble with reconciling sourcemaps with webpack stats for the main chunk. Both tools show the correct total size, and source maps presented me with chunk sizes that are equal to the total. However for webpack-bundle-analyzer I am getting this double effect as mentioned originally and the sum doesn't really add up to the expected bundle size. I will create an issue in webpack-bundle-analyzer.

@valscion
Copy link
Contributor

valscion commented Jun 6, 2018

Is webpack-contrib/webpack-bundle-analyzer#161 a non-issue in this context then? I was testing this in CRA and was only able to get some discrepency in a very limited respect (ie under the exact same conditions).

I think it is an issue, but I'm not sure if it is a big issue or something we can ignore. Let's continue discussion about that here: webpack-contrib/webpack-bundle-analyzer#188

However for webpack-bundle-analyzer I am getting this double effect as mentioned originally and the sum doesn't really add up to the expected bundle size. I will create an issue in webpack-bundle-analyzer.

That doesn't sound right. Let's discuss this in an issue over at https://github.com/webpack-contrib/webpack-bundle-analyzer ☺️

I haven't seen such a case myself and I don't recall us seeing a similar bug report before.

@generalov
Copy link

I added webpack-bundle-analyzer to my pet project last week. I spent several hours invetigating "tree-shaking issue" because the analyzer displays all modules from ramda package on the output. Luckily I've just found your discussion, thanks you guys. :)

webpack-bundle-analyzer does excellent work in terms of visualization, but visualization of the tree-shaked bundles is misleading now.

@valscion
Copy link
Contributor

Hi all! The issue with showing unrelated modules due to "tree shaking issue" should be no more 😊. Latest version (v3) now hides the contents of tree-shaken modules by default.

There's a checkbox saying the results might be inaccurate to toggle back to the original behavior.

Let me know if there's still something I could help with

@Timer Timer modified the milestones: 2.0.x, 2.x Sep 26, 2018
@gaearon
Copy link
Contributor Author

gaearon commented Sep 27, 2018

I'm happy to keep it if

  • It reports sizes of react/react-dom accurately
  • It doesn't make build significantly slower by default

If that's the case let's have this in 2.0. If we later add another way to customize things like this we can remove direct support but I don't want to take this away until then.

@Timer will test this

@Timer Timer modified the milestones: 2.x, 2.0.0 Sep 27, 2018
@Timer
Copy link
Contributor

Timer commented Sep 28, 2018

@samccone has there been progress on using bundle-buddy as a general purpose visual explorer when there is no duplication?

The proposed solution from January looks very promising.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2018

I think what we can do is that we keep this functionality but we don't include it in the documentation until the bundle visualizer hides stat size or adds something like "(likely inaccurate)" to it.

Then we don't have to block the release on this.

@Timer Timer modified the milestones: 2.0.0, 2.x Sep 28, 2018
@Timer
Copy link
Contributor

Timer commented Sep 28, 2018

Punting to 2.x, then.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2018

Should we revert the docs?

@Timer
Copy link
Contributor

Timer commented Sep 28, 2018

We removed the docs in #4564. Will probably just leave them out until Bundle Buddy is ready.

@valscion
Copy link
Contributor

What is "stat size" in the bundle analyzer output?

It's the original module sizes before any tree-shaking, dead code elimination or minification.

For some reason it's shown first even though it looks like a Webpack-specific size that has little to do with reality.

We'll discuss this with @th0r. It might make sense to not show it as the first value.

For react-dom, it's reported to be 135 kB. But production react-dom is <100 kB, so this can't be possible.

Yes, stat size can be confusing. We (the maintainers of webpack-bundle-analyzer) will look into it.

Again, even though it provides a real number later, this kind of stuff is why we've been avoiding webpack-specific tools, and unfortunately the fact that a wrong number shows up first even after all this discussion is making me anxious.

Very good point. Thank you for sharing your concerns.

It feels like the tools that use webpack data as an input just can't get this right, and keep inflating the size of React in measurements.

I understand the frustration. We'll see if we can resolve this in a way that would make it clear for beginners as well what the sizes refer to.

As an aside, "stat size" is probably not a very helpful description to a regular user. If I didn't know that the webpack API had "stats" object I wouldn't know what to make of it.

Yeah, I hear you. We might be able to come up with a better text for that.

I think what we can do is that we keep this functionality but we don't include it in the documentation until the bundle visualizer hides stat size or adds something like "(likely inaccurate)" to it.

Then we don't have to block the release on this.

This seems like a great solution as the situation is currently.

This would allow us over at webpack-bundle-analyzer to document on our repository, how things would work with CRA, and so I'd suspect confusion over documentation would be directed towards our library instead of CRA. What do you think?

@th0r
Copy link

th0r commented Sep 28, 2018

@gaearon

What is "stat size" in the bundle analyzer output?

Yeah, seems like I've picked the wrong name - I don't like it as well 😉 How about Original size? We can rename Parsed size too. Bundled size? Actual size?

For some reason it's shown first even though it looks like a Webpack-specific size that has little to do with reality.

What order do you feel is right? Parsed / Gzipped / Stat?

It feels like the tools that use webpack data as an input just can't get this right

I don't agree here. Nothing can give you as much information as the bundler itself and I can't see any reason why we can't rely on it. Yes, it can't give you all the info e.g. sizes of modules after minification, but we can use the provided data and pinch of magic to reliably fill the gaps.

I mean, why you can rely on tools that generate sourcemaps but can't rely on webpack that generate stats file? This example by @bugzpodder shows that sourcemaps are not very reliable source.

@Timer
Copy link
Contributor

Timer commented Sep 28, 2018

Yeah, seems like I've picked the wrong name - I don't like it as well 😉 How about Original size? We can rename Parsed size too. Bundled size? Actual size?

It's not the original size:

joes-mbp:js joe$ du -h ../../../node_modules/react-dom/cjs/react-dom.production.min.js
 96K    ../../../node_modules/react-dom/cjs/react-dom.production.min.js

What order do you feel is right? Parsed / Gzipped / Stat?

Gzipped, then Parsed. Remove Stat all together.

@th0r
Copy link

th0r commented Sep 28, 2018

It's not the original size

This tab shows the original sizes of modules that were used by webpack to create a bundle.

Gzipped, then Parsed. Remove Stat all together.

There are situations when you don't have resulting bundle files (only stats.json) and analyzer can only show original module sizes, reported by webpack. Maybe Initial size?

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2018

Maybe “Size before minification” and put it at the end? Or “Source code size”.

Thanks for being so responsive! I’m sure we can find a good solution.

@th0r
Copy link

th0r commented Sep 28, 2018

Maybe “Size before minification” and put it at the end?

The thing is the output bundle may not be minified.

Or “Source code size”.

I like it! What about Parsed size? Maybe create a poll in Twitter? 😁

@valscion
Copy link
Contributor

valscion commented Sep 28, 2018

What about Parsed size?

Could it be Compiled size? Or Output size?

@Janpot
Copy link
Contributor

Janpot commented Sep 28, 2018

What's the benefit of knowing this stat size? Is there anything helpful to derive from it? I feel like @Timer makes a good point, to just remove it altogether. In case there are no resulting bundle files, just say so, "no bundle file for this module" or something.

@valscion
Copy link
Contributor

valscion commented Sep 28, 2018

Is there anything helpful to derive from it?

I'll quote @th0r:

There are situations when you don't have resulting bundle files (only stats.json) and analyzer can only show original module sizes, reported by webpack.


But in case we do have parsed and gzip sizes, should we then skip showing the stats sizes altogether? Or hide showing the stat size behind another checkbox (or alike) with more text telling the size is inaccurate? Like we do when showing the tree-shaken contents:

screen shot 2018-09-28 at 14 53 26

EDIT: Sorry, I re-read your comment @Janpot and only now discovered you said this:

In case there are no resulting bundle files, just say so, "no bundle file for this module" or something.

We'd still like to keep the existing feature possible (showing only results from stats.json without output files) so this solution isn't a perfect one for us.

@valscion
Copy link
Contributor

valscion commented Sep 28, 2018

Btw, here's a performance build for public consumption we generated from our application today, using the latest webpack-bundle-analyzer v3.0.2

https://s3.eu-west-1.amazonaws.com/perf-reports.venuu.fi/1538120089-report.html

EDIT: Note that the size of react-dom is large because we're still using React 15.x version. The size reported is accurate.

@th0r
Copy link

th0r commented Sep 28, 2018

What's the benefit of knowing this stat size? Is there anything helpful to derive from it?

The first thing that comes to my mind is that you can see how well the module is minified/compressed relative to it's original (source) size.

@Janpot
Copy link
Contributor

Janpot commented Sep 28, 2018

The first thing that comes to my mind is that you can see how well the module is minified/compressed relative to it's original (source) size.

  1. in this case, maybe showing a percentage would be more helpful, and easier to label?
  2. this argument only works when there's a gzipped and/or parsed size to begin with, so if there's none of these, showing the stat size maybe isn't adding anything helpful either?

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2018

“Parsed size” is definitely not a good way to refer to what’s currently known as “stat size”.

In the performance sensitive community “parsed size” means “size of code that is being parsed by the browser”. It has no relation to how much webpack needs to parse.

@valscion
Copy link
Contributor

In the performance sensitive community “parsed size” means “size of code that is being parsed by the browser”.

This is indeed what the current "parsed size" means. I think @th0r's question was what should we name it instead of "parsed size" (as it is currently named)

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2018

“Original size” and reorder them: gzip first, then parsed, then original.

?

@th0r
Copy link

th0r commented Sep 28, 2018

Ok, I've created a poll to pick a better name: https://twitter.com/grunin_ya/status/1045661352608628736
@gaearon could you retweet it to cover larger audience?

@th0r
Copy link

th0r commented Sep 28, 2018

@gaearon and here is for Parsed size: https://twitter.com/grunin_ya/status/1045663101599854593

@generalov
Copy link

bundled, compressed, original ?

@bugzpodder
Copy link

#6438 and most recent version of source-map-explorer allows us to analyze all chunks now, so we decided to remove the --stats functionality in v3.

@lock lock bot locked and limited conversation to collaborators Mar 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants