-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add a public bundledPackages/excludedExternals option to DEWP
#45948
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
base: trunk
Are you sure you want to change the base?
Conversation
to exclude dependencies from externalizing.
to match more accurately to the actual behavior.
|
|
|
@gziolo Would you mind taking a look? |
| /** | ||
| * External dependencies | ||
| */ | ||
| import { isEmpty } from 'lodash'; |
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.
Note that those imports might need an ESLint ignore now that we're fully removing Lodash in #52571
| import { isEmpty } from 'lodash'; | |
| // eslint-disable-next-line no-restricted-imports | |
| import { isEmpty } from 'lodash'; |
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 heads-up.
I changed lodash to react-dom in the test fixture (6c0863c). I think it's more sustainable and future-proof to test against something we plan to support in the longer term instead of silencing warnings.
instead of lodash. Avoid warnings and prepare for dropping lodash completely. Addresses https://github.com/WordPress/gutenberg/pull/45948/files/e0d164967659fea61f769f876aa0c63010fbda62#r1262670308
Output and assets file paths and names are constructed from the `output` config, which may be different from just the `entrypoint`.
2d70ceb to
6c0863c
Compare
|
@tyxla Thank you for the review. I merged the Would you mind taking another look? |
|
@tomalec I mostly wanted to chime in regarding the Lodash usage as I noticed it during my audit for open PRs and issues that introduced lodash. Unfortunately, I'm not very familiar with the dependency extraction plugin so I might not be the best one to help review this one. |
What?
This PR allows users of the Dependency Extraction Webpack Plugin "do a simple thing easy" (and complex things still possible) and exclude packages from externalizing in a simple way. It also upstream WooCommerceDEWP feature (woocommerce/woocommerce-admin#7034). Or, in other words, publish the private feature of
BUNDLED_PACKAGES.It also includes a tiny clarification in the README (e8f3e85). The example was using the
entrypointfor the assets file, whereas the plugin usesoutputconfig for that. (fixes #49872)Why?
requestToExternalis used to filter a list of packages.WooCommerceDependencyExtractionWebpackPlugin'sbundledPackagesnot working for@wordpress/packages.How?
This PR changes the DEWP constructor to take
excludedExternalsoption to use it to filter packages processed inexternalizeWpDepsbefore anyrequestToExternalis called.Turns the old
BUNDLED_PACKAGESto the option's default value and exposes it as a static property to allow extending it by consumers.Testing Instructions
Run automated tests
npx wp-scripts test-unit-js --config test/unit/jest.config.js packages/dependency-extraction-webpack-plugin/Manual testing
@wordpress/components,@wordpress/url,@wordpress/iconsand@wordpress/interface@wordpress/url,@wordpress/iconsand@wordpress/interfacegets bundled, and none of them is logged.Screenshots or screencast
Additional info:
bundledPackagesname. This would keep the old name, and make it directly compatible with existing WooCommerce implementation. However:options.requestToExternal, so this way we avoid breaking change./iconsand/interfaceare bundled, but didn't manage for test build to process it. I guess it's related to the monorepo setup. Forlodashit works as expected.cc @gziolo as you reviewed my latest DEWP related issues
cc @louwie17 as someone who implemented it in Woo, and could give a context about why it eventually landed as
bundledPackagesnotexcludedExternals.