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

Allow bundles to be analyzed with Webpack-specific tools #3945

Merged
merged 2 commits into from
Jun 4, 2018

Conversation

joshwcomeau
Copy link
Contributor

Associated issue: #1858

To analyze Webpack bundles, a "stats" JSON is required.

This PR allows that file to be created and saved to the build
directory, so that users can use it with Webpack-specific insight
tools like webpack-bundle-analyzer without ejecting their
application.

Updated the README to include details for how to do this.

Test plan

1. check that the stats flag works

I ran yarn build --stats in the root directory of the project. Once it completed, I verified that build/bundle-stats.json existed. I dragged it into Webpack Visualizer to check that the file is correct:

screen shot 2018-01-31 at 8 46 57 am

2. check that it works in a created project

I ran yarn create-react-app test to generate a new app.

After cd-ing into the directory and installing its dependencies, I followed the steps I added to the the README for using webpack-bundle-analyzer. Specifically I ran:

$ npm install --save webpack-bundle-analyzer

Then, I opened the project's package.json and added a script:

"analyze": "npm run build -- --stats && webpack-bundle-analyzer build/bundle-stats.json",

Finally, I ran the new script:

$ npm run analyze

The project built, and a new browser window automatically opened, with the bundle stats:
screen shot 2018-01-31 at 8 42 14 am

Copy link
Contributor Author

@joshwcomeau joshwcomeau left a comment

Choose a reason for hiding this comment

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

Annotations

@@ -56,6 +57,10 @@ if (!checkRequiredFiles([paths.appHtml, paths.appIndexJs])) {
process.exit(1);
}

// Process CLI arguments
const argv = process.argv.slice(2);
const writeStatsJson = argv.indexOf('--stats') !== -1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if stats is the best name... Open to suggestions!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think --stats is quite nice, as it pairs well with webpack --stats flag.


Webpack can produce a JSON manifest that details the bundles, and several tools can use that file to do analysis.

Unlike with sourcemaps, the JSON file isn't created automatically on build. You must pass a `--stats` flag, using `--` to ensure it's passed to `react-scripts`:
Copy link
Contributor Author

@joshwcomeau joshwcomeau Jan 31, 2018

Choose a reason for hiding this comment

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

The last bit of the second sentence in this paragraph feels like it's too "inside baseball"; like we shouldn't need to mention what's happening with react-scripts under the hood.

But if I omit it, I worry that we aren't explaining the --, and this might cause folks to not notice it. I can imagine people being frustrated when npm run build --stats doesn't work and they can't figure out why?

Maybe there's a happy middle-ground. Open to copy suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a nice, succinct explanation to me. I don't have trouble with mentioning react-scripts here, maybe @gaearon has ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think -- is needed with Yarn actually, maybe it's worth to mention...

Copy link
Contributor

Choose a reason for hiding this comment

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

True. With yarn, we actually get a deprecation warning when using --. It might be a good idea to note it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the process to merge this PR? From what I see the only required changes are on the README.md file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think everything after the word flag can be removed. There's an example of the full command on the next line.

@@ -1972,6 +1980,44 @@ npm run build
npm run analyze
```

### Using Webpack Stats JSON
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a note that this feature is available only in some specific version of react-scripts, like many other parts of the README have?

Note: this feature is available with react-scripts@x.y.z and higher.

I don't know how the process of adding these version notes goes, though. Are they added just before releasing a new version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good idea!

I'm also unfamiliar with the process, since yeah I don't know which version it'll be released in. Happy to update the PR if this is knowable in advance, though :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to end up in react-scripts 2.0 so it's probably a good idea to include that in the README. I'm actually not 100% on the process for this either. I don't think these notes are generated automatically so you might as well add it yourself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this part or hide it behind comments. 2.x is the main branch now so that's what you find when you look at the repository. It will be deeply confusing to the users to see this in README now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@valscion
Copy link
Contributor

valscion commented Feb 5, 2018

So cool! Thanks for working on this, it looks great to me! 💞

Copy link

@langpavel langpavel left a comment

Choose a reason for hiding this comment

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

Nice change, but I don't want new dependency

@@ -33,6 +33,7 @@ const path = require('path');
const chalk = require('chalk');
const fs = require('fs-extra');
const webpack = require('webpack');
const bfj = require('bfj');

Choose a reason for hiding this comment

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

Big Friendly JSON. Is this really needed?

return bfj
.write(paths.appBuild + '/bundle-stats.json', stats.toJson())
.then(() => resolve(resolveArgs))
.catch(error => reject(new Error(error)));

Choose a reason for hiding this comment

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

As I can see you don't really need BFJ because you just write only one chunk.
fs.writeFile(..., JSON.strigify(stats)) should be more effective by my opinion.

Choose a reason for hiding this comment

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

So, this was a suggestion in the issue #1858, for concerns that with very large bundles, JSON.stringify could run out of memory and crash the node process. This was reported in webpack-bundle-analyzer, where they use JSON.stringify(). bfj is streaming, so it should avoid this potential issue.

Hi @joshwcomeau, thanks for fast reply.
So if I understand, it will traverse object prepared for serialization, write it by converting text piece by piece, potentially wasting more CPU, time and memory too, but it uses small bits of memory which can be collected by GC during processing.

@joshwcomeau
Copy link
Contributor Author

joshwcomeau commented Feb 15, 2018

Hi @langpavel!

So, this was a suggestion in the issue #1858, for concerns that with very large bundles, JSON.stringify could run out of memory and crash the node process. This was reported in webpack-bundle-analyzer, where they use JSON.stringify(). bfj is streaming, so it should avoid this potential issue.

Let me know how you'd like me to proceed.

Copy link

@langpavel langpavel left a comment

Choose a reason for hiding this comment

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

Ok, changed my mind, BFJ may be reasonable here...

@joshwcomeau
Copy link
Contributor Author

Awesome, thanks!

Copy link

@trevorwhealy trevorwhealy left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@valscion
Copy link
Contributor

Anything I could do to help this PR getting merged, @gaearon? The implementation is solid and docs are good enough for the first pass. I don't see any blockers here.

@kamranayub
Copy link

Would be great to see this merged, went looking for this feature today 👍

@joshwcomeau
Copy link
Contributor Author

Any updates on this? I understand that it's one of many open PRs, but I'd love to see this get merged.

@iansu
Copy link
Contributor

iansu commented May 27, 2018

I remember the issue discussing this but I didn't realize there was a PR this far along. I can take a look at it and test it out in the next couple of days.

@iansu
Copy link
Contributor

iansu commented May 28, 2018

This looks good to me, aside from the couple minor README changes I suggested. Based on the discussion in the linked issue it sounds like @gaearon is onboard with adding this feature so I'm happy to merge it once the docs are updated.

To analyze Webpack bundles, a "stats" JSON is required.

This PR allows that file to be created and saved to the `build`
directory, so that users can use it with Webpack-specific insight
tools like `webpack-bundle-analyzer` without ejecting their
application.

Updated the README to include details for how to do this.
@joshwcomeau
Copy link
Contributor Author

Updated the branch based on @iansu's comments, and rebased to deal with the conflict.

Thanks, excited to see this make it into 2.0 =)


The quickest way to get insight into your bundle is to drag and drop that JSON file into [Webpack Visualizer](https://chrisbateman.github.io/webpack-visualizer/).

Another very popular tool is [`webpack-bundle-analyzer`](https://www.npmjs.com/package/webpack-bundle-analyzer).
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we link to the GitHub page instead? https://github.com/webpack-contrib/webpack-bundle-analyzer

The README gets a bit squished on the https://www.npmjs.com/package/webpack-bundle-analyzer page 😕. In my opinion, the GitHub rendered page looks nicer.

In any case, this is only a nitpick and can be kept as-is ☺️

+ "analyze": "npm run build -- --stats && webpack-bundle-analyzer build/bundle-stats.json",
"start": "react-scripts start",
"build": "react-scripts build",
"build:with-stats": "react-scripts build",
Copy link
Contributor

@valscion valscion May 31, 2018

Choose a reason for hiding this comment

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

Should this build:with-stats line also have a + in the diff code example? Or should we keep the build:with-stats out from this example altogether?

Also, this example doesn't use the --stats flag correctly so it might even be incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Yeah, this is old. Will update.

@valscion
Copy link
Contributor

So amazing to see this soon getting finished! Yay! Thank you for continuing working on this PR, @joshwcomeau 💞

@joshwcomeau
Copy link
Contributor Author

@valscion made the requested changes. Good catch on build:with-stats being unnecessary!

@audiolion
Copy link

Looking forward to this! thanks for the work @joshwcomeau 👍

Copy link
Contributor

@valscion valscion left a comment

Choose a reason for hiding this comment

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

Looks great to me! 👍

@bugzpodder bugzpodder merged commit a98c3df into facebook:next Jun 4, 2018
@Timer Timer added this to the 2.0.0 milestone Jun 4, 2018
@gaearon
Copy link
Contributor

gaearon commented Jun 5, 2018

I haven't been following closely so I'll want to take another look at this before we ship 2.0.

In general I agree with #1858 (comment).

But since a lot of people want this I don't think it hurts to add this (but we might remove it in the future).

@gaearon
Copy link
Contributor

gaearon commented Jun 5, 2018

I filed #4563 for the follow-up.

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
Allow bundles to be analyzed with Webpack-specific tools
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.