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

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

Closed
gziolo opened this issue Apr 5, 2019 · 6 comments · Fixed by #15124
Closed
Assignees
Labels
[Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.

Comments

@gziolo
Copy link
Member

gziolo commented Apr 5, 2019

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: Automattic/wp-calypso#31513. It's an improved version of how Gutenberg handles external scripts in webpack config which can be found here:

/**
* 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
// externals to reduce the size of generated bundles.
//
// See: https://github.com/WordPress/gutenberg/issues/13890
'@babel/runtime/regenerator': 'regeneratorRuntime',
},
wordpressExternals,
];

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:

function gutenberg_register_packages_scripts() {
$packages_dependencies = include dirname( __FILE__ ) . '/packages-dependencies.php';
foreach ( $packages_dependencies as $handle => $dependencies ) {
// Remove `wp-` prefix from the handle to get the package's name.
$package_name = strpos( $handle, 'wp-' ) === 0 ? substr( $handle, 3 ) : $handle;
$path = "build/$package_name/index.js";
gutenberg_override_script(
$handle,
gutenberg_url( $path ),
array_merge( $dependencies, array( 'wp-polyfill' ) ),
filemtime( gutenberg_dir_path() . $path ),
true
);
}
}

It should be possible to make it fully automated with the webpack plugin discussed.

@gziolo gziolo added [Type] Enhancement A suggestion for improvement. Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality [Tool] WP Scripts /packages/scripts labels Apr 5, 2019
@gziolo gziolo changed the title Automate updating the list of JavaScript modules when registering script with PHP Automate updating the list of JavaScript modules when registering scripts with PHP Apr 5, 2019
@gziolo gziolo changed the title Automate updating the list of JavaScript modules when registering scripts with PHP Automate updating the list of dependencies when registering JavaScript scripts with PHP Apr 5, 2019
@simison
Copy link
Member

simison commented Apr 5, 2019

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: Automattic/jetpack#11591

@gziolo
Copy link
Member Author

gziolo commented Apr 5, 2019

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:

gutenberg_override_script(
$handle,
gutenberg_url( $path ),
array_merge( $dependencies, array( 'wp-polyfill' ) ),
filemtime( gutenberg_dir_path() . $path ),
true
);

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Apr 10, 2019
@gziolo gziolo self-assigned this Apr 10, 2019
@gziolo gziolo removed the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Apr 17, 2019
@gziolo
Copy link
Member Author

gziolo commented Apr 17, 2019

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.

@sirreal
Copy link
Member

sirreal commented Apr 17, 2019

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:

if ( window.postboxes.page !== postType ) {
window.postboxes.add_postbox_toggles( postType );

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

  • Should they be part of the default handlers?
  • If not part of the defaults, should the plugin include opt-in functionality to handle these scripts?

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
Copy link
Member Author

gziolo commented Apr 18, 2019

@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
Copy link
Member

aduth commented Apr 23, 2019

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' )
    )
);

@gziolo gziolo removed [Status] In Progress Tracking issues with work in progress labels Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants