Skip to content
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 a public bundledPackages/excludedExternals option to DEWP #45948

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

tomalec
Copy link
Contributor

@tomalec tomalec commented Nov 21, 2022

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 entrypoint for the assets file, whereas the plugin uses output config for that. (fixes #49872)

Why?

  1. To allow doing a simple thing easy, as in the majority of DEWP's use-cases, I've seen requestToExternal is used to filter a list of packages.
  2. To reduce the confusion of a non-Woo specific (at least conceptually) behavior of providing a simple list of packages to filter, not basing available in WP version of DEWP.
  3. To reduce the confusion of WooCommerceDependencyExtractionWebpackPlugin's bundledPackages not working for @wordpress/ packages.

How?

This PR changes the DEWP constructor to take excludedExternals option to use it to filter packages processed in externalizeWpDeps before any requestToExternal is called.
Turns the old BUNDLED_PACKAGES to the option's default value and exposes it as a static property to allow extending it by consumers.

Testing Instructions

Run automated tests

  1. npx wp-scripts test-unit-js --config test/unit/jest.config.js packages/dependency-extraction-webpack-plugin/

Manual testing

  1. Create a project that uses @wordpress/components, @wordpress/url, @wordpress/icons and @wordpress/interface
  2. Setup webpack config to use
new DependencyExtractionWebpackPlugin( {
	requestToExternal( package ) { console.log( package ) },
	excludedExternals: [
		...DependencyExtractionWebpackPlugin.excludedExternals,
		'@wordpress/url'
	],
} ),
  1. Confirm that @wordpress/url, @wordpress/icons and @wordpress/interface gets bundled, and none of them is logged.

Screenshots or screencast

Additional info:

  1. I was considering keeping the bundledPackages name. This would keep the old name, and make it directly compatible with existing WooCommerce implementation. However:
    1. IMO, it's not the accurate name, as those packages may not get bundled at all. For example, if none of them is actually used & imported.
    2. The behavior is slightly different than in Woo, as the package will not reach options.requestToExternal, so this way we avoid breaking change.
    3. We can still have a direct migration path or alias for the old name.
  2. I was trying to write tests to check that the default exclusions /icons and /interface are bundled, but didn't manage for test build to process it. I guess it's related to the monorepo setup. For lodash it works as expected.
  3. Initially I added it explicitly as
     static excludedExternals = // ...
    but eslint fails to parse it. Probably due to the old ES version used.

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 bundledPackages not excludedExternals.

to exclude dependencies from externalizing.
to match more accurately to the actual behavior.
@tomalec tomalec requested a review from gziolo as a code owner November 21, 2022 16:12
@codesandbox
Copy link

codesandbox bot commented Nov 21, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@tomalec
Copy link
Contributor Author

tomalec commented Dec 5, 2022

@gziolo Would you mind taking a look?

/**
* External dependencies
*/
import { isEmpty } from 'lodash';
Copy link
Member

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

Suggested change
import { isEmpty } from 'lodash';
// eslint-disable-next-line no-restricted-imports
import { isEmpty } from 'lodash';

Copy link
Contributor Author

@tomalec tomalec Jul 14, 2023

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.

Output and assets file paths and names are constructed from the `output` config, which may be different from just the `entrypoint`.
@tomalec
Copy link
Contributor Author

tomalec commented Jul 14, 2023

@tyxla Thank you for the review. I merged the trunk here, addressed your comment, and add a small improvement to the README.

Would you mind taking another look?

@tyxla
Copy link
Member

tyxla commented Jul 14, 2023

@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.

@tomalec
Copy link
Contributor Author

tomalec commented Jul 14, 2023

Ok, thanks :)

@gziolo @louwie17 Would you mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEWP docs and implementation mismatch/confusion in regards of assets file name
2 participants