-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add analyze-bundles
script
#21827
Add analyze-bundles
script
#21827
Conversation
a30380f
to
6b99217
Compare
Size Change: +4.98 kB (0%) Total Size: 821 kB
ℹ️ View Unchanged
|
@youknowriad or @aduth – I think we had it before but it was removed for some reason. Does it ring a bell? |
I do vaguely recall something like this a very long time ago. My impression is that it's not much of a concern if it's totally optional. Maybe it was removed if it wasn't providing much value and was causing some trouble? I'll see if I can dig it up. |
There was #10185. It was moved from the root In fact, it still exists! gutenberg/packages/scripts/config/webpack.config.js Lines 73 to 75 in 2f83264
But if I recall (because I debated it 😄), we moved away from reusing this shared configuration in Gutenberg, so we probably don't have it available currently via the environment variable. I think if we're to reintroduce it, we should find some way which reuses the configuration from |
As far as I can tell, the |
Yes, that's largely what I was expecting in referring to "we moved away from reusing this shared configuration in Gutenberg". There was some legitimate case for Gutenberg having very particular needs that motivated the change to a completely custom configuration, though it was still disappointing that we couldn't reconcile our own needs with a configuration we were otherwise touting as being a "common" configuration. What I mean by "find some way which reuses", in other cases we've promoted to merge some or all of the configuration. It would be nice if we could do something like that here, in order to avoid duplicating this configuration. (Aside: It's also duplicated in a way which isn't consistent, e.g. there is no My recommendation would be:
|
This makes it easier to analyze bundles with webpack-bundle-analyzer, which is already included as a project dependency.
1d21738
to
99fac3b
Compare
Thanks, @aduth! I gave this a go, and I couldn't find any great options for merging configuration. The biggest issue there is the plugins, as you pointed out. Since it's an array of instanced plugin objects, you lose all information regarding what's in there. So your only options become to either replace the plugins array entirely, or to accept everything in it and just append your own plugins at the end. If plugins could be an object instead (which it can't; I tried), that would neatly solve the issue by allowing us to name specific plugins to find and replace them if needed. The other option would be to export a plugins object separately, in something like a named export, but that's not supported in CommonJS. As such, all I can do is make things a bit more consistent between both webpack configs :-/ |
99fac3b
to
601a6ef
Compare
Follows the default behaviour of webpack
optimization: { | ||
// Only concatenate modules in production, when not analyzing bundles. | ||
concatenateModules: | ||
mode === 'production' && ! process.env.WP_BUNDLE_ANALYZER, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a relevant entry we should mention in scripts/CHANGELOG.md
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an entry 👍
@@ -3,6 +3,7 @@ | |||
### Breaking Changes | |||
|
|||
- The bundled `puppeteer` (`^2.0.0`) dependency has been replaced with `puppeteer-core` in version `3.0.0`. Puppeteer uses Chromium v81 instead of Chromium v79. See the [full list of changes](https://github.com/puppeteer/puppeteer/releases/tag/v3.0.0). It also allowed preventing Chromium installation together with `@wordpress/scripts`. It happens now on-demand when running `test-e2e` script, and it re-triggers only when a new version is required. | |||
- Bundle analysis now runs with module concatenation disabled. This represents the size of individual modules more accurately, at the cost of not providing an exact byte-for-byte match to the final size in the production chunk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you merged this change without rebasing. It ended up in the section that was already published to npm:
https://github.com/WordPress/gutenberg/blob/master/packages/scripts/CHANGELOG.md#900-2020-04-30
Also, it doesn't look like a breaking change, or is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I did do that, yes. Whether it's a breaking change or not depends on interpretation; if we consider the bundle analysis to be part of the scripts
package "user contract", then this is a breaking change, otherwise it's not.
Shall I prepare a PR moving this to the right section and making it a non-breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be great 👍
This makes it easier to analyse bundles with
webpack-bundle-analyzer
, which is already included as a project dependency.This generates colourful and helpful graphics like this one:
Description
Add a new script,
analyze-bundles
, as well as the instrumentation required for it inwebpack.config.js
.How has this been tested?
This should not affect any code, only the build process, which appears to still work normally when using any other script.
Test by running
npm run analyze-bundles
.Types of changes
New feature involving build changes only.
Checklist: