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

Webpack dependency plugin: Document unsupported multiple instances #15451

Merged
merged 4 commits into from May 7, 2019

Conversation

@sirreal
Copy link
Member

commented May 6, 2019

Description

Multiple instances of the @wordpress/dependency-extraction-webpack-plugin produce unexpected results:

webpack.config.plugins = [
const defaultConfig = require('@wordpress/scripts/config/webpack.config');
const config = {
   new DependencyExtractionWebpackPlugin({
      injectPolyfill: true,
  }),
   new DependencyExtractionWebpackPlugin({
      useDefaults: false,
  }),
   new DependencyExtractionWebpackPlugin({
      requestToExternal( request ) { return 'foobar'; }
  }),
]

Each of the above plugin instances has different options and different expectations. This is problematic mostly because each of the plugin instances will attempt to produce the same output, colliding and overwriting with unexpected results. While the above example is contrived, the following shows a case where multiple instances are created and are less obvious, from #15430 (comment):

const defaultConfig = require('@wordpress/scripts/config/webpack.config');
const config = {
  ...defaultConfig,
  plugins: [
    ...defaultConfig.plugins,
    new DependencyExtractionWebpackPlugin( {
      requestToExternal: ( request ) => {},
      requestToHandle: ( request ) => {},
    } ),
  ],
};

Add documentation discouraging multiple instances and provide instructions about removing the default instance from scripts.

Question

While updating the documentation is helpful, it does not prevent painful bugs from being introduced. It's straightforward for the plugin to detect multiple instances, should the plugin throw an error?

diff --git a/packages/dependency-extraction-webpack-plugin/index.js b/packages/dependency-extraction-webpack-plugin/index.js
index f99ec61fa..9e8e613de 100644
--- a/packages/dependency-extraction-webpack-plugin/index.js
+++ b/packages/dependency-extraction-webpack-plugin/index.js
@@ -75,6 +75,19 @@ class DependencyExtractionWebpackPlugin {
 	}
 
 	apply( compiler ) {
+		// Multiple instances of this plugin is unsupported. Warn if detected.
+		if (
+			compiler.options.plugins.filter(
+				( plugin ) => this !== plugin && plugin.constructor.name === this.constructor.name
+			).length
+		) {
+			throw new Error(
+				`Multiple instances of the webpack plugin ${ this.constructor.name } found.\n` +
+					'This is unsupported and may produce unexpected results.\n' +
+					'See [some-link].'
+			);
+		}
+
 		this.externalsPlugin.apply( compiler );
 
 		const { output } = compiler.options;

Janitorial changes included in this PR

  • Update local binding in README to DependencyExtractionWebpackPlugin, better matching package name
  • Fix requestToHandle inline comment in README.

How has this been tested?

Recommended documentation was tested locally.

Types of changes

Documentation

Checklist:

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

sirreal added some commits May 6, 2019

@gziolo
Copy link
Member

left a comment

While updating the documentation is helpful, it does not prevent painful bugs from being introduced. It's straightforward for the plugin to detect multiple instances, should the plugin throw an error?

Is it something that other webpack plugins would do? We should follow what webpack core plugins do.

( plugin ) => this !== plugin && plugin.constructor.name === this.constructor.name

Would the above work properly when a package consumer would import the plugin under a custom name?

@sirreal

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

While updating the documentation is helpful, it does not prevent painful bugs from being introduced. It's straightforward for the plugin to detect multiple instances, should the plugin throw an error?

Is it something that other webpack plugins would do? We should follow what webpack core plugins do.

( plugin ) => this !== plugin && plugin.constructor.name === this.constructor.name

I don't think this is common in webpack for primarily because webpack doesn't have a built-in config sharing story, although it's being explored. You set up a webpack config for a project and you don't typically include the same plugin twice. Additionally, most plugins don't add an asset to be written like this plugin does where the one instance may conflict with another. An exception is the SourceMapDevToolPlugin, which inspired the asset addition for this plugin. It doesn't check for duplicate plugins.

Is there any recommendation for how the @wordpress/scripts webpack config should be reused? If the usage from #15430 (comment) is to be expected, we should come up with an appropriate way to handle it.

Would the above work properly when a package consumer would import the plugin under a custom name?

Yes, if you try to build with this patch, it errors:

diff --git a/packages/scripts/config/webpack.config.js b/packages/scripts/config/webpack.config.js
index 5a3b30163..37c8f4ba1 100644
--- a/packages/scripts/config/webpack.config.js
+++ b/packages/scripts/config/webpack.config.js
@@ -8,7 +8,8 @@ const path = require( 'path' );
 /**
  * WordPress dependencies
  */
-const DependencyExtractionWebpackPlugin = require( '@wordpress/dependency-extraction-webpack-plugin' );
+const FooPlugin = require( '@wordpress/dependency-extraction-webpack-plugin' );
+const BarPlugin = require( '@wordpress/dependency-extraction-webpack-plugin' );
 
 /**
  * Internal dependencies
@@ -67,7 +68,8 @@ const config = {
 		// WP_LIVE_RELOAD_PORT global variable changes port on which live reload works
 		// when running watch mode.
 		! isProduction && new LiveReloadPlugin( { port: process.env.WP_LIVE_RELOAD_PORT || 35729 } ),
-		new DependencyExtractionWebpackPlugin( { injectPolyfill: true } ),
+		new FooPlugin( { injectPolyfill: true } ),
+		new BarPlugin( { useDefaults: false } ),
 	].filter( Boolean ),
 	stats: {
 		children: false,

In that example, I matched constructor names (strings) because I was concerned about different versions of the plugin package being used and that name will likely be stable. An alternative would be ( plugin ) => this !== plugin && plugin instanceof this.constructor, but that would miss different versions of the plugin.

@gziolo

gziolo approved these changes May 7, 2019

Copy link
Member

left a comment

Let's start with documentation revised and tackle error handling separately :)

@gziolo gziolo added this to the 5.7 (Gutenberg) milestone May 7, 2019

@@ -59,7 +81,6 @@ By default, the following module requests are handled:
| `@babel/runtime/regenerator` | `regeneratorRuntime` | `wp-polyfill` |
| `@wordpress/*` | `wp['*']` | `wp-*` |
| `jquery` | `jQuery` | `jquery` |
| `jquery` | `jQuery` | `jquery` |

This comment has been minimized.

Copy link
@gziolo

gziolo May 7, 2019

Member

I noticed a duplicated line in README and included in this PR as well to avoid opening another PR.

@sirreal sirreal merged commit 9ad5b93 into master May 7, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@sirreal sirreal deleted the update/dependency-extraction-no-multiple-instances branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.