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

Package: Webpack plugin to externalize WordPress deps #31513

Merged
merged 20 commits into from
Apr 5, 2019

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Mar 15, 2019

Introduce a webpack plugin as described in #31486
Partly resolves Automattic/jetpack#11907

Automattic/jetpack#11591 demonstrates how the output of this plugin would be used:

$script_relative_path = 'path/to/script.js';
$script_deps_path     = 'path/to/script.deps.json';
$script_dependencies = file_exists( $script_deps_path )
	? json_decode( file_get_contents( $script_deps_path ) )
	: array();
$view_script = plugins_url( $script_relative_path, __FILE__ );
wp_enqueue_script( 'script', $view_script, $script_dependencies );

Example usage:

// Webpack config
{
  // …
  plugins: [
    new WordPressExternalDependenciesPlugin(),
  ]
}

// entry.js
import { Component } from '@wordpress/element';

// output.js
// …whatever that webpack magic looks like
const Component = wp.element;

// output.deps.json
[ 'wp-element' ]

Closes #31486

To do

  • ❌ Shall we always include wp-polyfill or make that an option? (handle in follow-up)

Testing

I've put the plugin into use for the Jetpack blocks build. The entry points like editor.js should produce editor.deps.json.

  • npx lerna run prepare --scope="*/o2-blocks" - is an editor.deps.json built to the same location as editor.js?
  • Test in Gutenberg following these instructions. They can be adapted for use in Jetpack as well.
  • Is a .deps.json file created for each entry point?
  • Do the deps look correct? You can compare them with our manually managed lists:
  • The blocks should continue to work exactly as before. @wordpress, jquery, react, react-dom, moment, and lodash deps should be externals that appear in exactly the same way as before.

@matticbot
Copy link
Contributor

@sirreal sirreal self-assigned this Mar 15, 2019
@simison
Copy link
Member

simison commented Mar 18, 2019

What do you think about including other non-wp.* externals shipped in WordPress here?

List used by Gutenberg:

https://github.com/WordPress/gutenberg/blob/6e340d6a4dbd0daa9d459e09592200ddcb676eb2/packages/scripts/config/webpack.config.js#L46-L64

const externals = [
	{
		react: 'React',
		'react-dom': 'ReactDOM',
		moment: 'moment',
		jquery: 'jQuery',
		lodash: 'lodash',
		'lodash-es': 'lodash',

		// Distributed NPM packages may depend on Babel's runtime regenerator.
		// In a WordPress context, the regenerator is assigned to the global
		// scope via the `wp-polyfill` script. It is reassigned here as an
		// externals to reduce the size of generated bundles.
		//
		// See: https://github.com/WordPress/gutenberg/issues/13890
		'@babel/runtime/regenerator': 'regeneratorRuntime',
	},
	wordpressExternals,
];

@simison
Copy link
Member

simison commented Mar 18, 2019

What do you think about including other non-wp.* externals shipped in WordPress here?

Related naive question; should we then need to worry about aliases such as lodash-eslodash?

https://github.com/WordPress/gutenberg/blob/6e340d6a4dbd0daa9d459e09592200ddcb676eb2/packages/scripts/config/webpack.config.js#L85-L89

@sirreal
Copy link
Member Author

sirreal commented Mar 18, 2019

What do you think about including other non-wp.* externals shipped in WordPress here?

Related naive question; should we then need to worry about aliases such as lodash-eslodash?

https://github.com/WordPress/gutenberg/blob/6e340d6a4dbd0daa9d459e09592200ddcb676eb2/packages/scripts/config/webpack.config.js#L85-L89

We should reasonably be able to cover a lot of cases and it's good to keep them in mind. It concerns me how varied lodash imports might look:

  • lodash
  • lodash/map
  • lodash-es
  • lodash-es/map?
  • lodash.map
  • lodash/fp (likely we won't support this)
  • lodash/fp/map (likely we won't support this)

I'd like to focus on supporting the basics on a first pass. In the future we can improve this.

@sirreal sirreal force-pushed the add/package-wp-extern-plugin branch from 3c48157 to 14148d4 Compare March 18, 2019 15:16
@sirreal sirreal marked this pull request as ready for review March 18, 2019 16:55
@sirreal sirreal force-pushed the add/package-wp-extern-plugin branch from b01ea5c to 25eb15a Compare March 18, 2019 17:24
@sirreal

This comment has been minimized.

@sirreal sirreal requested review from a team March 18, 2019 18:05
@sirreal sirreal added Jetpack [Goal] Gutenberg Working towards full integration with Gutenberg Gutenberg core [Type] Enhancement [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 18, 2019
ockham
ockham previously approved these changes Mar 18, 2019
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jon, awesome job! 🎉

Shall we always include wp-polyfill or make that an option?

Not sure. Might be fine to always include it for a first iteration and potentially add the option later if it turns out that we need it. (We'd need to add the polyfill tho before actually using the deps files in JP, i.e. before merging Automattic/jetpack#11591.)

The jetpack-blocks output isn't the same and I believe it's because additional things are externed like react and moment.

I tend to agree, from a cursory glance at the diff. The blocks I've tested seem to work well -- if there's anything off, I'm confident that we'll discover it before the next release.

@sirreal
Copy link
Member Author

sirreal commented Mar 19, 2019

I tried running this plugin in the Gutenberg webpack config and it exposed some issues. I've added some hacks that make the synthetic .deps.json asset look like a chunk or module so that an appropriate path can be built.

I'd be open to more idiomatic ways of creating a chunk. Source map plugins should provide some insight, but my comprehension of webpack internals is still quite limited.

To try it agains Gutenberg, you'll need to link this package (npm link from the package dir and npm link @automattic/wordpress-external-dependencies-plugin from the Gutenberg root) and apply the patch below to Gutenberg. Then npm install and npm run build.

Gutenberg patch
diff --git a/packages/scripts/config/webpack.config.js b/packages/scripts/config/webpack.config.js
index 9a1daabf5..516f6a907 100644
--- a/packages/scripts/config/webpack.config.js
+++ b/packages/scripts/config/webpack.config.js
@@ -4,54 +4,16 @@
 const { BundleAnalyzerPlugin } = require( 'webpack-bundle-analyzer' );
 const LiveReloadPlugin = require( 'webpack-livereload-plugin' );
 const path = require( 'path' );
+const WordPressExternalDependenciesPlugin = require("@automattic/wordpress-external-dependencies-plugin");
 
 /**
  * Internal dependencies
  */
 const { camelCaseDash, hasBabelConfig } = require( '../utils' );
 
-/**
- * Converts @wordpress/* string request into request object.
- *
- * Note this isn't the same as camel case because of the
- * way that numbers don't trigger the capitalized next letter.
- *
- * @example
- * formatRequest( '@wordpress/api-fetch' );
- * // { this: [ 'wp', 'apiFetch' ] }
- * formatRequest( '@wordpress/i18n' );
- * // { this: [ 'wp', 'i18n' ] }
- *
- * @param {string} request Request name from import statement.
- * @return {Object} Request object formatted for further processing.
- */
-const formatRequest = ( request ) => {
-	// '@wordpress/api-fetch' -> [ '@wordpress', 'api-fetch' ]
-	const [ , name ] = request.split( '/' );
-
-	// { this: [ 'wp', 'apiFetch' ] }
-	return {
-		this: [ 'wp', camelCaseDash( name ) ],
-	};
-};
-
-const wordpressExternals = ( context, request, callback ) => {
-	if ( /^@wordpress\//.test( request ) ) {
-		callback( null, formatRequest( request ), 'this' );
-	} else {
-		callback();
-	}
-};
 
 const externals = [
 	{
-		react: 'React',
-		'react-dom': 'ReactDOM',
-		moment: 'moment',
-		jquery: 'jQuery',
-		lodash: 'lodash',
-		'lodash-es': 'lodash',
-
 		// Distributed NPM packages may depend on Babel's runtime regenerator.
 		// In a WordPress context, the regenerator is assigned to the global
 		// scope via the `wp-polyfill` script. It is reassigned here as an
@@ -60,7 +22,6 @@ const externals = [
 		// See: https://github.com/WordPress/gutenberg/issues/13890
 		'@babel/runtime/regenerator': 'regeneratorRuntime',
 	},
-	wordpressExternals,
 ];
 
 const isProduction = process.env.NODE_ENV === 'production';
@@ -111,6 +72,7 @@ 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 WordPressExternalDependenciesPlugin(),
 	].filter( Boolean ),
 	stats: {
 		children: false,

@@ -0,0 +1,75 @@
const wpModules = new Map( [
[ 'jquery', { dependency: 'jquery', globalName: 'jQuery' } ],
[ 'lodash', { dependency: 'lodash', globalName: 'lodash' } ],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More lodash alternatives?

Suggested change
[ 'lodash', { dependency: 'lodash', globalName: 'lodash' } ],
[ 'lodash', { dependency: 'lodash', globalName: 'lodash' } ],
[ 'lodash-es', { dependency: 'lodash', globalName: 'lodash' } ],

@@ -0,0 +1,75 @@
const wpModules = new Map( [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to hear thoughts about this… it's practical but rigid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not generate @wordpress/ deps instead of defining each one manually, since their naming patterns are predictable and there seems to be a kind of contract that they'll remain to be so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that there are @wordpress/ packages that aren't available as script dependencies in WordPress and would need to be bundled. It may be that those packages are not for the browser and this bundler shouldn't need to worry about them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be an interesting configuration point, allow consumers to specify the elements of this map so if they're designing a plugin that exposes additional WordPress script dependencies, they can define them here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It very impractical because there will be more packages created and published and you will have to remember to update it.

@simison simison requested a review from gziolo March 19, 2019 09:05
@gziolo
Copy link
Member

gziolo commented Mar 19, 2019

I don't quite understand why you need to manually craft this list of dependencies. You probably could auto-generate it based on npm dependencies added to your package.json file or with a similar trick. This one of the examples of how we automate the maintenance of packages in Gutenberg:
https://github.com/WordPress/gutenberg/blob/master/webpack.config.js#L26-L28

@jsnajdr
Copy link
Member

jsnajdr commented Mar 19, 2019

The Gutenberg trick in particular will probably stop working as soon as some @wordpress/* dependency is transitive. For example, our package.json can specify a @wordpress/data dependency, but the fact that it further depends on @wordpress/api-fetch is hidden.

We need a better trick 🙂

@sirreal sirreal force-pushed the add/package-wp-extern-plugin branch from 886550d to 78787f7 Compare April 4, 2019 19:39
@sirreal
Copy link
Member Author

sirreal commented Apr 4, 2019

Rebased to fix conflicts.

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and tested that this works well with SDK builder, too:

npm run sdk -- generic --global-wp packages/o2-blocks/src/editor.js ./extern-test.js

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually one thing I just noticed;

in this branch, building with calypso-build gives me a new cache folder that isn't gitignored:

packages/calypso-build/.cache

Just run npx lerna run prepare --scope="*/o2-blocks" to replicate.

@sirreal sirreal merged commit 5cb5171 into master Apr 5, 2019
@sirreal sirreal deleted the add/package-wp-extern-plugin branch April 5, 2019 07:54
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 5, 2019
@sirreal
Copy link
Member Author

sirreal commented Apr 5, 2019

in this branch, building with calypso-build gives me a new cache folder that isn't gitignored:

I believe that's from a stale branch point. It was fixed on master by #32040.

@simison
Copy link
Member

simison commented Apr 5, 2019

Confirmed, all good in master. Thanks! :-)

ockham added a commit that referenced this pull request Apr 5, 2019
Mostly for Automattic/jetpack#11802, which needs the package to contain #31513.
Sticking with alpha versioning for a bit longer since there are a few more changes we know we'd like to get in before considering its interface somewhat stable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg Packages [Type] Enhancement
Projects
None yet
7 participants