Avoid including a nodejs library to reduce the index.js bundle size by 42% #1073
Conversation
eason9487
added
the
type: technical debt - dependency extraction
|
😲 |
|
Nice finding and great analysis. Thanks, @eason9487 🙏 📜 However, the one thing that struck me while WordPress/gutenberg#35630 (comment) is that we should probably not bundle any I Will test this PR today, as I think it's a great step forward, reducing our tech debt, by updating deps, and making merchants happier, by reducing bundle size and load. |
Ohh, thanks for reminding this! I kept this in mind but forgot to mention it in this PR at all. 🤦♂️ Indeed, a final solution would be to import them via DEWP for the long term if we could find a proper practice, but It looks like there is still a long way to go. So I was trying to reduce the size in advance before we got there. 😆 |
|
💅 📜 I wonder if we could remove now, those webpack lines But we may as well leave it for extraction of |
That's really a thing to keep in mind, or ... a note. 😆 Since we might still need the workaround if we bump up |
Changes proposed in this Pull Request:
Found this issue when looking into #1068 (comment).
@woocommerce/dataversion to 1.2.0 to avoid bundling the nodejscryptointo index.jsAdditional details
Root cause
When looking into the result of bundle analysis, I notice that the obvious
bn.jsappears several times and takes up a significant file size. After some investigations, it turns out the root cause is that@woocommerce/data1.1.1 imports the nodejs built-in librarycrypto, and it doesn't set thesideEffectsoption to enable the more advanced tree shaking. Therefore, all its dependencies are included into index.js, the GLA doesn't need it though.The overall unnecessary codes are highlighted by red background:
Solution
Fortunately, the issue has been addressed by woocommerce/woocommerce-admin#5768. And we have
@woocommerce/datarelevant changes these days:woocommerceTranslationused in title breadcrumb #1055So currently, we have three places using the
@woocommerce/data:NAMESPACEfor concatenating the API endpoint. The constant was there two years ago.google-listings-and-ads/js/src/reports/products/async-requests.js
Line 15 in e5cf5bd
getItemsselector for checking where a product is a variant.google-listings-and-ads/js/src/reports/products/products-report-filters.js
Lines 102 to 104 in e5cf5bd
getSettingselector for getting store's contry.google-listings-and-ads/js/src/hooks/useStoreCountry.js
Lines 19 to 22 in e5cf5bd
After specifying
@woocommerce/datato version 1.2.0 and checking:npm run test-unitand all JS unit tests are passedpackage-lock.jsonandnode_modules. Runnpm installandnpm start. No errors occurred when reinstalling and running npm modules from scratch.cryptocodes are disappeared. (See the Screenshots section.)I think the problems we encountered in PR #693 are no longer present.
Other possible options
@woocommerce/data, but there're many dependency tree changes inpackage-lock.json. Even though they could resolve the issue as well, I don't have much confidence to bump up@woocommerce/dataonly without related dependencies together.cryptoto an empty object by adding the below webpack config. But I think this approach is less ideal.Other details
Diff of included module files
Screenshots:
Detailed test instructions:
npm installto update node modules/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&subpath=%2Ffree-listings%2Fedit&pageStep=2)USaudience is selected in step 1npm run env -- NODE_ENV=production wp-scripts build --webpack-bundle-analyzerto start the analyzercryptorelevant codes. I used a regex to check this:Changelog entry