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.

Sign up for GitHub

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

Jump to bottom

Read script dependencies from generated files #15124

Merged
merged 8 commits into from
Apr 30, 2019
Merged

Read script dependencies from generated files #15124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
906250e
Rework script registration to use generated deps
sirreal Apr 23, 2019
993c8a8
Rework translation without packages-dependencies.php
sirreal Apr 24, 2019
f540605
Deprecate static list of dependencies
sirreal Apr 23, 2019
33991d6
Include build/*/*.deps.json in plugin zip
sirreal Apr 24, 2019
2c3b654
Add script dependencies undetectable by build tools
sirreal Apr 24, 2019
27bccbe
Add `wp-polyfill` dependency via webpack plugin
sirreal Apr 24, 2019
df21c64
Improve deprecated message
sirreal Apr 24, 2019
6de12cc
Remove obsolete lib/packages-dependencies.php
sirreal Apr 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/build-plugin-zip.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ npm run build
php bin/generate-gutenberg-php.php > gutenberg.tmp.php
mv gutenberg.tmp.php gutenberg.php

build_files=$(ls build/*/*.{js,css} build/block-library/blocks/*.php)
build_files=$(ls build/*/*.{js,css,deps.json} build/block-library/blocks/*.php)

# Generate the plugin zip file.
status "Creating archive... 🎁"
Expand Down
47 changes: 36 additions & 11 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,14 @@ function gutenberg_override_translation_file( $file, $handle ) {
return $file;
}

// Only override script handles generated from the Gutenberg plugin.
$packages_dependencies = include dirname( __FILE__ ) . '/packages-dependencies.php';
if ( ! isset( $packages_dependencies[ $handle ] ) ) {
// Ignore scripts whose handle does not have the "wp-" prefix.
if ( 'wp-' !== substr( $handle, 0, 3 ) ) {
return $file;
}

// Ignore scripts that are not found in the expected `build/` location.
$script_path = gutenberg_dir_path() . 'build/' . substr( $handle, 3 ) . '/index.js';
if ( ! file_exists( $script_path ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unsure how to test whether this is working well, I'd appreciate some help.

Copy link
Member

Choose a reason for hiding this comment

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

That's a great question. Travis seems to be happy about changes applied 🤷‍♂️

How about we compare all script tags in the HTML output between master and this branch?

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 is rather tricky to verify, but it seems to be working. Here's what I did:

  • Build the plugin zip from this PR.
  • Get a new WordPress site.
  • Install the plugin from the zip.
  • Change the site language.
  • Go to updates and install language updates. This will download the missing language files for the Gutenberg plugin we just uploaded.
  • Open the editor.
  • Check a known string: wp.i18n.__( 'Use the classic WordPress editor.' )
  • Find the plugin language file containing your string. In my case /wp-content/languages/plugins/gutenberg-es_ES-d83116db7e3459d9c9a90cae3216278f.json
  • Edit the language file changing the translation of your string (Utiliza el editor clásico de WordPress.) for something else (👋 Hi from the plugin language file.)
  • Activate the plugin.
  • Check the string from the editor again: wp.i18n.__( 'Use the classic WordPress editor.' )
  • Should be your new string.
  • Deactivate the plugin.
  • Back to the original string.

return $file;
}

Expand Down Expand Up @@ -177,17 +182,37 @@ function gutenberg_override_style( $handle, $src, $deps = array(), $ver = false,
* @since 4.5.0
*/
function gutenberg_register_packages_scripts() {
$packages_dependencies = include dirname( __FILE__ ) . '/packages-dependencies.php';
foreach ( glob( gutenberg_dir_path() . 'build/*/index.js' ) as $path ) {
Copy link
Member

Choose a reason for hiding this comment

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

Depending if and how similar code would make its way to core, I'm a bit sensitive to the performance implications of a glob.

Seems like there could be optimizations:

  • Avoiding the folder scan and pregenerating the scripts at build time (similar to generate-gutenberg-php.php)
  • Substituting glob with something faster (opendir)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍A nice enhancement for a future version or simply another part of the build would be to produce what is needed in PHP without needing to hit the filesystem or parse JSON.

This came up during the core-js slack chat.

CC: @gziolo

Copy link
Member

@gziolo gziolo Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Related conversation on WordPress Slack (link requires registration):

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

We didn't discuss file discovery which would be still an issue.

// Prefix `wp-` to package directory to get script handle.
// For example, `…/build/a11y/index.js` becomes `wp-a11y`.
$handle = 'wp-' . basename( dirname( $path ) );

// Replace `.js` extension with `.deps.json` to find the generated dependencies file.
$dependencies_file = substr( $path, 0, -3 ) . '.deps.json';

$dependencies = is_readable( $dependencies_file )
? json_decode( file_get_contents( $dependencies_file ) )
: array();

// Add dependencies that cannot be detected and generated by build tools.
switch ( $handle ) {
Copy link
Member

Choose a reason for hiding this comment

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

Works for me 🤷‍♂️

😅

case 'wp-block-library':
array_push( $dependencies, 'editor' );
break;

case 'wp-edit-post':
array_push( $dependencies, 'media-models', 'media-views', 'postbox' );
break;
}

// Get the path from Gutenberg directory as expected by `gutenberg_url`.
$gutenberg_path = substr( $path, strlen( gutenberg_dir_path() ) );

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 ),
gutenberg_url( $gutenberg_path ),
$dependencies,
filemtime( $path ),
true
);
}
Expand Down
265 changes: 0 additions & 265 deletions lib/packages-dependencies.php
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion packages/scripts/config/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,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 DependencyExtractionWebpackPlugin(),
new DependencyExtractionWebpackPlugin( { injectPolyfill: true } ),
Copy link
Member Author

Choose a reason for hiding this comment

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

Includes wp-polyfill as a script dependency for every entrypoint.

Copy link
Member

Choose a reason for hiding this comment

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

Includes wp-polyfill as a script dependency for every entrypoint.

Looking at the implementation, would wp-polyfill include wp-polyfill as a dependency of itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would yes.

That said, I don't see polyfill being built as part of Gutenberg (no build/polyfill/index.deps.json). Is this a theoretical concern or is the polyfill built somewhere we'd need to be careful with this?

Copy link
Member

@gziolo gziolo Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we will ever scan wp-polyfill but we can add guard clause to be on the safe side.

].filter( Boolean ),
stats: {
children: false,
Expand Down