WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.46k stars 4.18k forks source link

Automate updating the list of dependencies when registering JavaScript scripts with PHP #14837

Closed gziolo closed 5 years ago

gziolo commented 5 years ago

Shared initially on WordPress.org Slack (link requires registration at https://make.wordpress.org/chat/): https://wordpress.slack.com/archives/C5UNMSU4R/p1554453132137700

https://github.com/Automattic/wp-calypso/blob/78787f7af8ed8049ccc703f072a23f0c4f8de6a5/packages/wordpress-external-dependencies-plugin/README.md#wordpress

It would be great to backport work from the Jetpack team to Gutenberg and WordPress core. They implemented custom webpack plugin which generates JSON file with the list of WordPress script handles for every chunk generated. We wouldn’t have to maintain all those long lists of dependencies manually anymore. I will open an issue shortly. An example of usage in PHP:

$script_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();
$script_url = plugins_url( $script_path, __FILE__ );
wp_enqueue_script( 'script', $script_url, $script_dependencies );

Well, we probably could make this plugin more flexible and teach it to generate PHP file which returns an array as well for core specifically to avoid JSON decode dance.

All the code can be found in this PR: https://github.com/Automattic/wp-calypso/pull/31513. It's an improved version of how Gutenberg handles external scripts in webpack config which can be found here: https://github.com/WordPress/gutenberg/blob/244d7ceed9fb9d867175afdd965121e690f57856/packages/scripts/config/webpack.config.js#L13-L64

The idea is to copy the original webpack plugin to externalize WordPress dependencies and convert it to @wordpress/* package and then use it instead of the currently used code. Next step would be to update the way dependencies for packages are discovered. At the moment everything is hardcoded: https://github.com/WordPress/gutenberg/blob/1dcda6f70e23bc822b00800257d6893f42df5609/lib/client-assets.php#L179-L194 It should be possible to make it fully automated with the webpack plugin discussed.

simison commented 5 years ago

Note that question about how to handle wp-polyfill is still open at our end. We'll probably just always add it as a dependency to everything, to be safe.

PHP side to automated dependency loading in Jetpack: https://github.com/Automattic/jetpack/pull/11591

gziolo commented 5 years ago

Note that question about how to handle wp-polyfill is still open at our end. We'll probably just always add it as a dependency to everything, to be safe.

We took the same assumption in Gutenberg, we always include wp-polyfills as in reality 90% of modules would use it anyways:

https://github.com/WordPress/gutenberg/blob/1dcda6f70e23bc822b00800257d6893f42df5609/lib/client-assets.php#L186-L192

gziolo commented 5 years ago

There is this list of default scripts included and registered by WordPress

https://developer.wordpress.org/reference/functions/wp_enqueue_script/#default-scripts-included-and-registered-by-wordpress

It looks like Gutenberg uses indirectly 4 of them:

It's important to know, as they have to be included in the webpack automation which creates the list of dependencies which has just landed with #14869.

sirreal commented 5 years ago

There is this list of default scripts included and registered by WordPress

https://developer.wordpress.org/reference/functions/wp_enqueue_script/#default-scripts-included-and-registered-by-wordpress

It looks like Gutenberg uses indirectly 4 of them:

  • editor
  • postbox
  • media-models
  • media-views

It's important to know, as they have to be included in the webpack automation which creates the list of dependencies which has just landed with #14869.

We'll need to look at each one, but in general I'd like to see these leverage @wordpress/dependency-extraction-webpack-plugin from #14869. We can make the dependence explicit and apparent in code.

For example, postbox:

https://github.com/WordPress/gutenberg/blob/5430dced1e3594ca28f5a9c829aa9baac8905e27/packages/edit-post/src/store/effects.js#L41-L42

This could become:

import postboxes from 'postbox';
// …
if ( postboxes.page !== postType ) {
    postboxes.add_postbox_toggles( postType );

The webpack plugin can handle externalizing, adding the script handle to the dependency list, and the process becomes automated. Are these scripts ever expected to be used without being processed by the webpack build?

The downside would be that because postbox does not correspond to any "real" script, the build becomes dependent on some tool to make the postbox module external. That doesn't seem like a big problem at the moment because if the build ever does move away from webpack this will be one of many issues to deal with.

I do wonder how these "legacy" scripts should be handled going forward

I think it would be best to move ahead implementing custom handlers for Gutenberg to externalize this scripts as needed. If there is demand for making more legacy scripts available, we can figure out how best to address that when the need becomes clear.

gziolo commented 5 years ago

@youknowriad or @aduth - do you have some recommendations on how to handle those legacy dependencies? In general, this is an issue regardless as they are never listed for npm packages. The question is whether we are fine with that and patch it manually on the WordPress side in PHP files as it happens today? Or do we want to mark them as externals in webpack plugin and add some fancy logic to make it work?

aduth commented 5 years ago

This was discussed in today's Core JS meeting (link requires registration):

https://wordpress.slack.com/archives/C5UNMSU4R/p1556028213146100

From the discussion there, it was suggested as a short-term solution to continue referencing the window globals but (a) ensure that these references are guarded (to avoid errors in an npm context) and (b) merge these with the set of dependencies extracted by the Webpack plugin when registering a script. Long-term, we should seek to avoid references to window globals (see #15125)

Pseudo-code:

wp_enqueue_script(
    'my-script',
    plugins_url( 'script.js', __FILE__ ),
    array_merge(
        json_decode( file_get_contents( 'script.deps.json' ) ),
        array( 'postbox' )
    )
);