Add option for adding additional bundled packages in DependencyExtractionWebpackPlugin

[louwie17]

Description

Add option for adding additional bundled packages in DependencyExtractionWebpackPlugin, this will allow plugins to bundle a newer version of @wordpress/*, then what is available through window.wp.

How has this been tested?

• Checkout this branch run npm install and cd packages/dependency-extraction-webpack-plugin.
• npm link (so we can use it in another package)
• In a different directory pull a sample WP plugin, I used one of mine and ran npm install
• npm install @wordpress/dependency-extraction-webpack-plugin --save-dev
• Link the local version -> npm link @wordpress/dependency-extraction-webpack-plugin
• Add

  new DependencyExtractionWebpackPlugin({
    bundledPackages: ['@wordpress/components']
  }),
  

  Above WooCommerceDependencyExtractionWebpackPlugin in the webpack.config.js in plugins, and commenting out the WooCommerceDependencyExtractionWebpackPlugin.
• Run npm run build
• Check ./build/index.js, Button should be included in the bundle (you can search for function Button).

Types of changes

Adds a new option bundledPackages to DependencyExtractionWebpackPlugin, that takes an array of optional bundled dependencies, that will not be marked as external.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @louwie17! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[sirreal]

Hi @louwie17, thanks for the proposal! I have a few thoughts and questions.

I'm reluctant to make this easy to do in a broad way because there are packages we know don't behave well when duplicated. There are details in this issue and especially this comment #8981 (comment). With this plugin it should be easy and obvious to do things the "right way."

The purpose of this plugin is to avoid duplicating packages, which is generally a good thing. As an example, @wordpress/components is not a small package and including the entirety of it multiple times is harmful to the user experience.

I do believe I understand where this is coming from and might suggest an alternative. Nobody likes waiting for new features to be released as part of WordPress core 🙂

Rather than duplicating the entire plugin, what about copying just the new features into your plugin? You could even default to using the core implementation if provided. The duplicated code will still be included, but significantly less:

// Try to get it from the WordPress environment
import { NewFeature as CoreNewFeature } from '@wordpress/components';
// If not, use the copy included in my code
import { NewFeature as DuplicatedNewFeature } from './duplicated-new-feature.js';

const NewFeature = typeof CoreNewFeature !== 'undefined' ? CoreNewFeature : DuplicatedNewFeature;

@gziolo – does that align with your thinking?

[louwie17]

I'm reluctant to make this easy to do in a broad way because there are packages we know don't behave well when duplicated. There are details in this issue and especially this comment #8981 (comment). With this plugin it should be easy and obvious to do things the "right way."

The purpose of this plugin is to avoid duplicating packages, which is generally a good thing. As an example, @wordpress/components is not a small package and including the entirety of it multiple times is harmful to the user experience.

@sirreal Thanks for highlighting this, I wasn't as aware of this, and was mostly thinking about isolated components that wouldn't rely on other data packages. Including plugins that might only use a couple different components that would then be imported by tree-shaking.
As I think about that more, it should almost be on a component level, versus a package level.

I do believe I understand where this is coming from and might suggest an alternative. Nobody likes waiting for new features to be released as part of WordPress core 🙂

Yeah it was kind of two fold, something like this made it easier to test tree-shaking (as we recently enabled this in woocommerce/components - not yet released), and as mentioned above for plugin that relied on a few new components. I made a similar change in our woocommerce/dependency-extraction-webpack-plugin, but mostly for testing purposes, which in theory is kinda redundant.

Rather than duplicating the entire plugin, what about copying just the new features into your plugin? You could even default to using the core implementation if provided. The duplicated code will still be included, but significantly less:

I agree this could certainly work as an alternative.

Given your reasoning above, I would be fine foregoing this change, but could also add a strong warning when this config is being used (although this might easily be overlooked during a build process).

[sirreal]

The focus on tree-shaking is a little bit contrary to the goals of this plugin.

This plugin is meant to bundle nothing, but use the shared script dependencies that are provided by WordPress (or other plugins). It's a bridge between JavaScript modules and WordPress script dependencies.

JavaScript bundlers with tree-shaking focus on included only the necessary bits of a given package. My previous comment isn't entirely correct in that the bundled packages would likely benefit from tree-shaking and not be duplicated in their entirety.

As far as bundling (and tree-shaking) core packages, are you familiar with the useDefaults option? It can be set to false to disable all the default externalization and bundle everything. The plugin doesn't serve much purpose at that point 🙂

I also believe this was considered when implementing the plugin and you should be able to disable particular packages already without additional options. Have you tried something like this?

new DependencyExtractionWebpackPlugin({
  requestToExternal( request ) {
    // Override a given default by returning `false`
    if ( request === '@wordpress/components' ) {
      return false;
    }
  },
})

[louwie17]

I also believe this was considered when implementing the plugin and you should be able to disable particular packages already without additional options. Have you tried something like this?

Thanks, this would work well for testing purposes, I found the only down side with the above was that it would only use the custom requestToExternal, and there was no way of calling the original requestToExternal. So in order to just skip wordpress/components I would have to copy the rest of the logic from the default function.
It would be more useful if the default function was still run if the returned value of requestToExternal was undefined. Then your code example above would work well.

I will close this, as that would be a separate issue or PR, if you think that would be more beneficial?
Outside of that, there is enough alternatives for these changes not to be necessary, thanks for the discussion @sirreal