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

fix: call stats.toJson on each time to avoid the stats mutation #293

Merged
merged 1 commit into from Jul 29, 2019

Conversation

wood1986
Copy link
Contributor

@wood1986 wood1986 commented Jul 17, 2019

fix #292. But I need some help about testing these change.

@jsf-clabot
Copy link

jsf-clabot commented Jul 17, 2019

CLA assistant check
All committers have signed the CLA.

Copy link

@mistic mistic left a comment

Choose a reason for hiding this comment

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

LGTM

@wood1986
Copy link
Contributor Author

Hey @valscion and @th0r Do you have any updates?

@th0r
Copy link
Collaborator

th0r commented Jul 29, 2019

I'm a little worried about performance and memory aspect of this change as stats object can be huge. Is it possible to better find problematic places (which modify original stats object) and fix them instead?

@wood1986
Copy link
Contributor Author

I'm a little worried about performance and memory aspect of this change as stats object can be huge. Is it possible to better find problematic places (which modify original stats object) and fix them instead?

  1. I will not prefer this because it is not scale-able. Every time you introduce a new state mutation and you will need to undo it somewhere. Additionally, because of the Promise.all you modify the same object and it will still have a chance to break others.

  2. Currently, if people need the correct stat.json and there is a bug which will affect them when it is generated from this plugin, they will choose to re-run webpack with --json. The time of the re-run must be more than the performance that you are concerned.

  3. The plugin will only run a maximum of 2 tasks which are generateStatsFile and either startAnalyzerServer or generateStaticReport. Not sure how many people need 2 tasks and I do not think there are a lot. Here is the test from my app and my stats.json is about 50.4MB. I do not think 160.253 is a lot.

generateStatsFile: 160.253ms // stats.options = {}
generateStaticReport: 905.879ms

generateStatsFile: 13.481ms // stats.options = { children: false, chunks: false, modules: false, sources: false, reasons: false }
generateStaticReport: 964.424ms

@th0r
Copy link
Collaborator

th0r commented Jul 29, 2019

  1. I will not prefer this because it is not scale-able. Every time you introduce a new state mutation and you will need to undo it somewhere. Additionally, because of the Promise.all you modify the same object and it will still have a chance to break others.

What I'm talking about is find places that modify original stats object and rewrite them because it's a bug - they shouldn't do this.

2. Currently, if people need the correct stat.json and there is a bug which will affect them when it is generated from this plugin, they will choose to re-run webpack with --json. The time of the re-run must be more than the performance that you are concerned.

I'm sorry I don't quite follow.

3. I do not think 160.253 is a lot.

Where is this number came from? What are those logs about? Is first log represents your approach and the second one is the current implementation? Why does stats options differ then?

@wood1986
Copy link
Contributor Author

What I'm talking about is find places that modify original stats object and rewrite them because it's a bug - they shouldn't do this.

If you are still talking about one stats object does everything, I do not think I cannot because

stats = stats.toJson(this.opts.statsOptions);
. If I pass the trimmed stats to startAnalyzerServer or generateStaticReport, it cannot show the chunk details and it will only be a rectangle box with the name. I believe this is not expected result.

Here is the scenario I need, I want the plugin output the report with the chunk details. At the same time, I want the plugin output the stat.json which will remove some fields that the report need.

I'm sorry I don't quite follow.

What I am trying to say is this is the workaround that I am using to achieve the scenario I mentioned right before that. And I have to pay more to wait for the output. The performance and memory that you are concerned are not a lot. They are 160.253ms and 51MB of RAM

Where is this number came from? What are those logs about? Is first log represents your approach and the second one is the current implementation? Why does stats options differ then?

Both logs is after my fix.

160.253 comes from the statsOptions = {} in the worst case scenario which outputs everything. The first log is about when you specify statsOptions = {} you have to pay another 160.253ms to output the stats.json. If you apply the filter in statsOptions, you have to pay 13.481ms to output the stats.json.

@th0r
Copy link
Collaborator

th0r commented Jul 29, 2019

Ok, I finally understood what the problem is. Apologize for my dullness 😉

@th0r th0r merged commit fa2ac7b into webpack-contrib:master Jul 29, 2019
@th0r
Copy link
Collaborator

th0r commented Jul 29, 2019

Published in v3.4.0

@dosentmatter
Copy link

Just a note for anyone who sees this. I am using nextjs and I was trying to generate stats with webpack-bundle-analyzer, but I kept getting truncated JSON. As in JSON that just ends randomly and is invalid. I didn't try version 3.4.0 but 3.4.1 fixed it for me. I'm guessing 3.4.0 would have worked too because it had this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The stats.json generated via this Plugin has some fields which do not belong to webpack
6 participants