-
Notifications
You must be signed in to change notification settings - Fork 21
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
Externalize all @woocommerce
packages via DEWP and remove the selective bundling implementation
#2072
Conversation
…zes packages into DEWP.
👍 They also stay at those versions since our L-2 7.8.2:
Maybe we could consider (not necessarily here) bumping |
I'm not a native English speaker, but I passed In the past, we did bundle some packages provided by WordPress/WooCommerce instances. We did so to use a specific package version, for example, to ship a new feature we need, fix, or avoid a bug. However, bundling a package tends to introduce other errors, as some packages are not effectively modular, so we face version conflicts, style collisions, etc.
Also, we'd like to reduce the size of our bundle, so eventually, we aim to extract/externalize as much as possible and import from an external package when possible.
To help with that, we had implemented the `extracted/` prefix in the past. It was also a custom implementation in [`webpack.config.js`](webpack.config.js).
Thanks to that, even though a package is bundled, the given import would fetch it from external.
Now, we have already externalized all DEWP-able packages, so we also removed the implementation of the `extracted/` prefix.
If someday we ever need it again, please refer to:
- The PR implemented it: https://github.com/woocommerce/google-listings-and-ads/pull/1762
- The commit removed it: https://github.com/woocommerce/google-listings-and-ads/commit/5a2e20409a53ccb3b7fcbfe5c46988ca2b153b38
Alternatively, we can consider an opposite approach and selectively **bundle** with a similar prefix implementation. |
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 review, @tomalec
I tried bumping the WP and WC packages to L-2 while working on #2069, but there were quite a few problems with
Thanks for the suggestion! Updated in 0157e87. |
Changes proposed in this Pull Request:
Closes #1527
Closes #1971
This PR externalizes all
@woocommerce
packages via DEWP, and it can also fix #1971 by the way.@woocommerce
packages to use external sources from DEWP.@woocommerce/currency
@woocommerce/date
@woocommerce/number
@woocommerce/tracks
Screenshots:
Detailed test instructions:
npm start
,npm run dev
,npm run start:hot
andnpm run build
, can run successfully.npm run env -- NODE_ENV=production wp-scripts build --webpack-bundle-analyzer
@woocommerce/*
package included in the build bundle.moment.tz
gets conflicted when the site timezone is other than UTC #1971 to check ifwp.date.getDate()
can run successfully.Additional details:
The current versions of
@woocommerce/currency
,@woocommerce/date
,@woocommerce/number
and@woocommerce/tracks
packages are the same versions as the latest WooCoomerce release (8.0.2):Therefore, switching to DEWP should not have any effect.
Changelog entry