-
Notifications
You must be signed in to change notification settings - Fork 29
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
refactor(mc-scripts): manually group some vendor chunks #3615
Conversation
🦋 Changeset detectedLatest commit: ce14413 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
react: [ | ||
'react', | ||
'react-dom', | ||
'react-router', | ||
'react-router-dom', | ||
'react-intl', | ||
'@reduxjs/toolkit', | ||
'redux', | ||
'react-redux', | ||
'redux-logger', | ||
'redux-thunk', | ||
], |
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.
@emmenko the only change I made is to lump redux and react together; since redux is fairly small this seems fine to do.
@tylermorrisford by default the applications are built using webpack but we also have "experimental" support for Vite.
UPDATE: After some investigation, I think the problem is that we're trying to create bundles for dependencies that are transitive to the application. For instance, We can, for instance, create a bundle for the application-shell packages, but |
Building an application is failing for both To test it, you can get into the
I think we need to investigate a little further what the updated configuration actually does. |
Two items I've discovered in testing the Vite build process by linking the my local version of app-kit with the monorepo and building the account application:
|
@@ -181,7 +181,8 @@ | |||
"@typescript-eslint/parser": "^5.52.0" | |||
}, | |||
"patchedDependencies": { | |||
"[email protected]": "patches/[email protected]" | |||
"[email protected]": "patches/[email protected]", | |||
"[email protected]": "patches/[email protected]" |
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.
vite-bundle-analyzer
has a rollup
very flexible peer dependency (2.X -> 4.X) but it actually requires 3.X at least because of a typescript type usage.
Since we're using rollup 2.X in the repo (force by rollup
), the alternative I took is to change the type usage (very simple change)
@@ -77,6 +86,10 @@ async function run() { | |||
pluginSvgr(), | |||
pluginDynamicBaseAssetsGlobals(), | |||
pluginI18nMessageCompilation(), | |||
process.env.ANALYZE_BUNDLE === 'true' && |
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.
@@ -77,6 +86,10 @@ async function run() { | |||
pluginSvgr(), | |||
pluginDynamicBaseAssetsGlobals(), | |||
pluginI18nMessageCompilation(), | |||
process.env.ANALYZE_BUNDLE === 'true' && | |||
analyzer({ defaultSizes: 'parsed', openAnalyzer: true }), | |||
process.env.ANALYZE_BUNDLE_TREE === 'true' && |
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.
], | ||
}; | ||
|
||
const removeNonExistantDependencies = ( |
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.
This is just to make sure the packages we're defining in the manualChunks
map actually exists in the application to build.
This is a safety measure so in case one of them are remove (eg: moment) the build does not fail but will only emit a warning about 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.
Thanks for the comment. I was fearful we'd remove something else but those we specify at first.
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.
Great job, thanks for the contributions! 🙌
@@ -108,6 +109,7 @@ | |||
"thread-loader": "3.0.4", | |||
"url": "^0.11.0", | |||
"vite": "~4.5.3", | |||
"vite-bundle-analyzer": "0.12.1", |
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'm not sure if the patch will be used here when someone installs the mc-scripts
package.
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.
Fair question. Couldn't find documentation on it. I must say I assume it would be applied.
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 don't know either but it won't matter in this case: the change in the source code is about a Typescript type which is not generic in the rollup
version we're using but the dependency code expects it to be.
So, the generated code is exactly the same (no typings go there).
manualChunks: Record<string, string[]>, | ||
appDependencies: string[] | ||
) => { | ||
return Object.entries(manualChunks).reduce((acc, [chunkName, vendors]) => { |
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.
Nit: let's use a more meaningful name
return Object.entries(manualChunks).reduce((acc, [chunkName, vendors]) => { | |
return Object.entries(manualChunks).reduce((chunkGroups, [chunkName, vendors]) => { |
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.
Updated here: f8f062c
const existingVendors = vendors.filter((vendor) => | ||
appDependencies.includes(vendor) | ||
); | ||
if (existingVendors.length > 0) { |
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.
const existingVendors = vendors.filter((vendor) => | |
appDependencies.includes(vendor) | |
); | |
if (existingVendors.length > 0) { | |
const hasExistingVendors = vendors.some((vendor) => | |
appDependencies.includes(vendor) | |
); | |
if (hasExistingVendors) { |
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.
Let's picture this scenario:
const manualChunks = {
'chunk-A': ['dep-a', 'dep-b'];
}
With the current code, if the app does not have 'dep-b' anymore, the manual chunk would still be created with dep-a
. This would not work if we apply your suggestion.
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.
Thanks, nevermind then. I misread the code 🙈
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.
Thanks a lot for the effort on this work!
- [vite-bundle-analyzer](https://github.com/KusStar/vite-bundle-visualizer) | ||
- [rollup-plugin-visualizer](https://github.com/btd/rollup-plugin-visualizer) (tree visualization) | ||
|
||
You can use them by setting this environment variables" |
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.
You can use them by setting this environment variables" | |
You can use them by setting this environment variables: |
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.
Updated here: f8f062c
@@ -108,6 +109,7 @@ | |||
"thread-loader": "3.0.4", | |||
"url": "^0.11.0", | |||
"vite": "~4.5.3", | |||
"vite-bundle-analyzer": "0.12.1", |
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.
Fair question. Couldn't find documentation on it. I must say I assume it would be applied.
], | ||
}; | ||
|
||
const removeNonExistantDependencies = ( |
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.
Thanks for the comment. I was fearful we'd remove something else but those we specify at first.
Summary
This PR revives another PR created by @emmenko to manually group some vendor chunks in the
@commercetools-frontend/mc-scripts
package. Doing so gives us a few extra files in the bundle but a significantly smaller vendor chunk.Description
Using this method to test, I rebuilt the account application before and after making these changes; this resulted in the main app chunk being reduced from ~660KB to ~412KB. Based on this I feel that all monorepo and satellite applications would benefit.
UPDATE
We had to change a bit the packages we wanted to create independent bundles for.
We need to bear in mind that splitting is done at the application level, so what we can split are their direct dependencies so it's not possible to split internals of (for instance) the
application-shell
package (that's something we can do separately and internally to that package).We decided to create manual bundles for:
application-shell
andapplication-shell-connectors
moment
andmoment-timezone
@commercetools-uikit/icons
Building the playground application, here is what biggest files originally looks like (webpack):
and this is how it looks like with the new manual bundles:
Also, this is the comparison when using vite.
BEFORE
AFTER
We're also adding new vite plugins that will help to analyze the built bundles using this env variables:
ANALYZE_BUNDLE
ANALYZE_BUNDLE_TREE
This is how they look like: