Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 10 months ago

#45065 closed enhancement (fixed)

Include Gutenberg packages in WordPress core.

Reported by: omarreiss's profile omarreiss Owned by: omarreiss's profile omarreiss
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: General Keywords: fixed-5.0 has-patch
Focuses: javascript Cc:

Description

The Gutenberg JavaScript packages will continue to be maintained on Github and distributed through NPM.

To include the Gutenberg packages in WordPress we'll need to:

  • add them as dependencies in WordPress' package.json
  • build all the necessary packages using Webpack
  • register the build files so they can be enqueued. This unblocks moving the PHP over to WordPress core.

Attachments (10)

45065-wip.diff (846.1 KB) - added by atimmer 5 years ago.
45065-wip.2.diff (116.8 KB) - added by atimmer 5 years ago.
45065-wip.3.diff (119.9 KB) - added by atimmer 5 years ago.
45065-wip.4.diff (119.9 KB) - added by atimmer 5 years ago.
45065.diff (173.5 KB) - added by atimmer 5 years ago.
45065.2.diff (225.2 KB) - added by atimmer 5 years ago.
45065.3.diff (226.6 KB) - added by atimmer 5 years ago.
45065.4.diff (225.2 KB) - added by atimmer 5 years ago.
45065.5.diff (225.1 KB) - added by atimmer 5 years ago.
45065-script-loader.diff (16.5 KB) - added by atimmer 5 years ago.

Change History (56)

#1 @atimmer
5 years ago

In 43687:

Build tools: Combine webpack config files.

This prepares us for building the Gutenberg packages.

See #45065.

#2 @pento
5 years ago

  • Milestone changed from Awaiting Review to 5.0

#3 @atimmer
5 years ago

In 43688:

Build tools: Upgrade webpack to version 4.

  • Minification is done by uglify, so disable that in the media build.
  • The webpack boilerplate has changed, which explains the changes in the build files.
  • ModuleConcatenationPlugin is enable by default for production builds so we don't have to specify that ourselves.

See #45065.

This ticket was mentioned in Slack in #core-js by atimmer. View the logs.


5 years ago

@atimmer
5 years ago

#5 @atimmer
5 years ago

45065-wip.diff is a working patch that builds all available necessary Gutenberg packages from npm.

@atimmer
5 years ago

#6 @atimmer
5 years ago

45065-wip.2.diff doesn't include the media files with inline sourcemaps.

This ticket was mentioned in Slack in #core-js by netweb. View the logs.


5 years ago

@atimmer
5 years ago

@atimmer
5 years ago

@atimmer
5 years ago

#8 @atimmer
5 years ago

45065.diff is the latest status. I've changed the vendor handling to copy from node_modules instead of building them with a custom Webpack config.

After the missing packages have been published to npm I can commit this and continue work with the PHP registering of the assets.

PS. The discussion about this patch is located on GitHub: https://github.com/Yoast/wordpress-develop-mirror/pull/118. That makes it easier to comment on lines in the patch.

@atimmer
5 years ago

@atimmer
5 years ago

@atimmer
5 years ago

@atimmer
5 years ago

#9 @atimmer
5 years ago

In 43719:

Build tools: Build @wordpress packages with webpack.

We decided to split the media webpack config into it's own file. The
main webpack config then combines this file with the packages config.

Include vendor scripts by copying them. We copy the minified files if
they are available. If they aren't available we minify the original
files ourselves.

Props omarreiss, herregroen, gziolo, youknowriad, netweb, adamsilverstein.
See #45065.

#11 @SergeyBiryukov
5 years ago

Travis failures were caused by using an outdated Node version, fixed in [43721].

#13 @atimmer
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 43723:

Script loader: Register @wordpress scripts.

This allows the packages to be consumed by plugins and core itself.
The code has been based on the work done in the Gutenberg plugin.

We've added an array with all the packages and the vendor packages to
loop through. This sets a convention so all packages will be
registered in the same way. This array can eventually be generated by
a webpack plugin.

We need to register TinyMCE explicitly. Previously TinyMCE was used
by inserting custom <script> tags into the relevant admin pages.
This is not suitable for the new editor, so we need to explicitly
register TinyMCE. We could, in the future, refactor the custom
<script> tags to use the registered TinyMCE script instead.

Polyfills are inserted into the page only when necessary using
document.write.

Props omarreiss, herregroen, youknowriad, gziolo.
Fixes #45065.

#14 @youknowriad
5 years ago

#44987 was marked as a duplicate.

This ticket was mentioned in Slack in #core-privacy by azaozz. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-committers by pento. View the logs.


5 years ago

#17 @pento
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thank you for getting this done so quickly, @atimmer!

There are a few changes that we need to make here. Nothing major, just a bit of shuffling. 🙂

Instead of calling wp_default_packages_vendor(), wp_default_packages_scripts(), and wp_default_packages_inline_scripts() inside wp_default_scripts(), they should be attached to the wp_default_scripts action. wp_register_tinymce_scripts() should be attached to that action, too.

Is there any particular reason for wp_default_packages_vendor() and wp_default_packages_scripts() to be different functions? We don't differentiate between third party and WordPress scripts in wp_default_scripts(). They could be combined into a new wp_default_packages() function.

Similarly, wp_register_tinymce_scripts() could either be merged wp_default_packages().

wp_default_packages_inline_scripts() should be merged into wp_default_packages(), in the same style as inline scripts are added in wp_default_scripts().

There are several filters being run in wp_default_packages_inline_scripts() that don't have docblocks.

wp_register_tinymce_scripts() is a partial duplication of _WP_Editors::print_tinymce_scripts(): these need to be combined as much as possible, I suspect wp_register_tinymce_scripts() could really be merged into wp_default_packages() or wp_default_scripts(), too.

wp-tinymce-lists is trying to load index.js, it should be plugin.js.

All of the URL strings should be in the form "foo/bar/baz$suffix.js", rather than 'foo/bar/baz' . $suffix . '.js' or "foo/bar/baz{$suffix}.js".

I get where you were going with wp_default_packages_scripts() and wp_default_packages_vendor() being arrays of scripts that you then loop over to register, but this is probably going to cause maintenance headaches down the road. You can see in wp_default_scripts() how there are weird little variations in how scripts are loaded, a future version of WordPress will likely need to break these scripts out of the array, so we can do the same thing. Let's just do it the same way as wp_default_scripts().

Let's leave $script being accepted as a reference for now: it will probably go away as part of #44979, but that issue needs further investigation.

#18 @pento
5 years ago

[43688] added mode to the media build. This means that grunt webpack:dev causes all of the media-*.js files to be built in development mode, which is going to cause some huge patches to be accidentally submitted.

Let's remove that for now, we can really only make use of it when the full JS reorg is in place.

#19 @atimmer
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 43738:

Script loader: Adjust JS packages registration.

Adjusts the packages registration after [43723]:

  • Combine the different registration functions into one

wp_default_packages function. To reach this goal move the prefix
logic into a function so it can be called from different locations.
Use a static variable there to prevent duplicate inclusion of
version.php.

  • Call this function from the wp_default_scripts action by

registering it as a default filter.

  • Combine some of the logic in _WP_Editors::print_tinymce_scripts

into wp_register_tinymce_scripts. The logic to force an uncompressed
TinyMCE script file stays in _WP_Editors::force_uncompressed_tinymce
because that logic is very specific to the classic editor.

  • The script handle wp-tinymce is now a dependency of the editor

script handle. In combination with the previous item, this makes the
classic editor work.

  • Adjust the syntax of the script paths to be more consistent with

other WordPress code.

  • Always use "production" mode for the media files to prevent people

from inadvertently committing development files.

Props pento, omarreiss.
Fixes #45065.

#20 @atimmer
5 years ago

@pento I've processed your feedback in [43738].

I merged wp_default_packages_vendor, wp_register_tinymce_scripts, wp_default_packages_scripts and wp_default_packages_inline_scripts into wp_default_packages but kept the functionality in separate functions to keep a decent separation of concerns.

I chose to leave the array for now. I think it's actually a good thing that it discourages custom tweaks and invites better solutions. If tweaks really turn out to be necessary, we can always refactor.

If there's anything else to address I can commit that tomorrow.

#21 @azaozz
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[43738] introduces a possible regression. It changes the order of loading of tinymce.js in class-wp-editor.php (not related to script-loader or Gutenberg).

Before the changes do_action( 'before_wp_tiny_mce', self::$mce_settings ); was firing before tinymce.js was loaded. Now it fires after.

Similarly tinyMCEPreInit was defined before tinymce.js was loaded, now it's defined after. This is especially important as TinyMCE looks for tinyMCEPreInit to get some predefined settings, see https://github.com/tinymce/tinymce/blob/7dfd8102cadfb35e7304cacec7f122aa9e81b1ad/src/core/main/ts/api/EditorManager.ts#L195.

Both happen as tinymce.js is now always enqueued through script-loader, regardless of where it is used.

Both of these don't affect loading of TinyMCE in core, but may affect plugins. Since we are trying to be very careful not to break anything in the 5.0 branch, thinking that change should be reverted.

If we need to abstract some parts of class-wp-editor.php, lets do it, but thinking we shouldn't change any of the functionality there, at least not in 5.0 :)

Last edited 5 years ago by azaozz (previous) (diff)

#22 @atimmer
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 43753:

Script loading: Fix regression after [43738].

After [43738], TinyMCE would be loaded earlier than before, which
makes filters run at a different time relative to the loading of
TinyMCE. Fix this by calling wp_print_scripts at the location where
TinyMCE would previously be inserted as a <script> tag in the page.

Props azaozz, omarreiss.
Fixes #45065.

#23 @atimmer
5 years ago

@azaozz Thank you for noticing the regression. I've fixed it in [43753].

#24 @atimmer
5 years ago

In 43754:

Script loading: Fix a PHP error introduced in [43753].

See #45065.
Props swissspidy.

#25 @youknowriad
5 years ago

@atimmer Will this change cause wp-tinymce to not load in Gutenberg? (I mean it will because it's defined as a dependency elsewhere too but it feels like it's not an ideal fix).

Theory:

Imagine Gutenberg switches TinyMCE over something else in the other scripts while still loading the editor script for the classicBlock or something, it will break right?

I thought I'd raise but I understand that it might not be so simple and that right now, Gutenberg do depend on tinymce elsewhere.

#26 @SergeyBiryukov
5 years ago

  • Keywords fixed-5.0 added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging to trunk.

#27 @atimmer
5 years ago

@youknowriad The current state on 5.0 is compatible with the current state of 4.9.8. Which means that I don't expect it to break. Gutenberg calls the relevant _WP_Editors methods itself, so even if Gutenberg were to remove TinyMCE, the <script> tag would still be printed by wp_print_scripts in print_tinymce_scripts.

#28 @pento
5 years ago

In 43802:

Script Loader: Fix metadata being registered as an inline script.

[43723] included script metadata for the wp-polyfill script that was being registered as an inline script.

See #45065.

#29 @pento
5 years ago

In 43803:

Scripts: wp-url is a dependency of wp-api-fetch.

See #45065.

#30 @pento
5 years ago

In 43805:

REST API: Add endpoints for blocks.

WP_REST_Block_Renderer_Controller allows rendering of server-side rendered blocks, whilst WP_REST_Blocks_Controller allows retrieving of reusable blocks.

Props desrosj, danielbachhuber, pento.
See #45065.

This ticket was mentioned in Slack in #core-js by omarreiss. View the logs.


5 years ago

#32 @pento
5 years ago

In 43812:

Styles: Add helper functions for loading block styles.

Blocks are able to register styles that used in the editor and the frontend, or only in the editor. These functions ensure the correct styles are loaded in the correct place.

See #45065.

#33 @atimmer
5 years ago

In 43978:

Build tools: Combine webpack config files.

This prepares us for building the Gutenberg packages.

Merges [43687] to trunk.
See #45065.

#34 @atimmer
5 years ago

In 44111:

Build tools: Upgrade webpack to version 4.

  • Minification is done by uglify, so disable that in the media build.
  • The webpack boilerplate has changed, which explains the changes in the build files.
  • ModuleConcatenationPlugin is enable by default for production builds so we don't have to specify that ourselves.

Merge notes: In trunk uglify isn't run on the media files after webpack, so webpack does need to do that. Newer webpack versions use terser-webpack-plugin as the default minification. Use the uglifyjs-webpack-plugin plugin to maintain the same behavior as before. We can look into terser as a minifier later.

Merges [43688] to trunk.
See #45065.

#35 @atimmer
5 years ago

In 44112:

Build tools: Build @wordpress packages with webpack.

We decided to split the media webpack config into it's own file. The
main webpack config then combines this file with the packages config.

Include vendor scripts by copying them. We copy the minified files if
they are available. If they aren't available we minify the original
files ourselves.

Props omarreiss, herregroen, gziolo, youknowriad, netweb, adamsilverstein.
Merges [43719] to trunk.
See #45065.

#36 @desrosj
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44114:

Script loader: Register @wordpress scripts.

This allows the packages to be consumed by plugins and core itself.
The code has been based on the work done in the Gutenberg plugin.

We've added an array with all the packages and the vendor packages to
loop through. This sets a convention so all packages will be
registered in the same way. This array can eventually be generated by
a webpack plugin.

We need to register TinyMCE explicitly. Previously TinyMCE was used
by inserting custom <script> tags into the relevant admin pages.
This is not suitable for the new editor, so we need to explicitly
register TinyMCE. We could, in the future, refactor the custom
<script> tags to use the registered TinyMCE script instead.

Polyfills are inserted into the page only when necessary using
document.write.

Props omarreiss, herregroen, youknowriad, gziolo, atimmer.

Merges [43723] to trunk.

Fixes #45065.

#37 @desrosj
5 years ago

In 44115:

Script loader: Adjust JS packages registration.

Adjusts the packages registration after [43723]:

Combine the different registration functions into one wp_default_packages function. To reach this goal move the prefix logic into a function so it can be called from different locations. Use a static variable there to prevent duplicate inclusion of version.php.

Call this function from the wp_default_scripts action by registering it as a default filter.

Combine some of the logic in _WP_Editors::print_tinymce_scripts into wp_register_tinymce_scripts. The logic to force an uncompressed TinyMCE script file stays in _WP_Editors::force_uncompressed_tinymce because that logic is very specific to the classic editor.

The script handle wp-tinymce is now a dependency of the editor script handle. In combination with the previous item, this makes the classic editor work.

Adjust the syntax of the script paths to be more consistent with other WordPress code.

Always use "production" mode for the media files to prevent people from inadvertently committing development files.

Props pento, omarreiss, atimmer.

Merges [43738] into trunk.

Fixes #45065.

#38 @desrosj
5 years ago

In 44119:

Script loading: Fix regression after [43738].

After [43738], TinyMCE would be loaded earlier than before, which makes filters run at a different time relative to the loading of TinyMCE. Fix this by calling wp_print_scripts at the location where TinyMCE would previously be inserted as a <script> tag in the page.

Also, an TinyMCE translation related <script> that was mistakenly removed in [44115].

Props azaozz, omarreiss, swisspidy, atimmer.

Merges [43753], [43754] to trunk.

Fixes #45065.

#39 @pento
5 years ago

In 44143:

Script Loader: Fix metadata being registered as an inline script.

[43723] included script metadata for the wp-polyfill script that was being registered as an inline script.

Merges [43802] from the 5.0 branch to trunk.

See #45065.

#40 @jeremyfelt
5 years ago

In 44150:

REST API: Add endpoints for blocks.

WP_REST_Block_Renderer_Controller allows rendering of server-side rendered blocks, whilst WP_REST_Blocks_Controller allows retrieving of reusable blocks.

Merges [43805] and [43806] from the 5.0 branch to trunk.

Props desrosj, danielbachhuber, pento, Presskopp, swissspidy.
See #45065, #45098.

#41 @desrosj
5 years ago

In 44157:

Styles: Add helper functions for loading block styles.

Blocks are able to register styles that used in the editor and the frontend, or only in the editor. These functions ensure the correct styles are loaded in the correct place.

Props pento.

Merges [43812] to trunk.

See #45065.

This ticket was mentioned in PR #4356 on WordPress/wordpress-develop by @oandregal.


11 months ago
#42

  • Keywords has-patch added

Trac ticket https://core.trac.wordpress.org/ticket/45065

The wp_enqueue_registered_block_scripts_and_styles callback is bound to enqueue_block_editor_assets and enqueue_block_assets actions.

It doesn't need to, and it should be sufficient to only use enqueue_block_assets. By definition, that hook fires for both front and editor.

Originally introduced at https://github.com/WordPress/wordpress-develop/commit/76525a75b7e84a8a34aede1b8aef4aecb6623bf8

@oandregal commented on PR #4356:


11 months ago
#43

I ran a performance analysis and haven't detected this PR has any impact. It is still worthwhile to do it right.

---

For transparency, this is what I've done, so anyone can reproduce if interested.

  • Enable XDEBUG in profile mode using URLs for output. Set the debug variables to production. This is the diff:

{{{diff
diff --git a/.env b/.env
index 63a8169f64..789e9871c6 100644
--- a/.env
+++ b/.env
@@ -18,7 +18,7 @@ LOCAL_DIR=src

LOCAL_PHP=latest


# Whether or not to enable Xdebug.

-LOCAL_PHP_XDEBUG=false
+LOCAL_PHP_XDEBUG=true

##
# The Xdebug features to enable.

@@ -31,7 +31,7 @@ LOCAL_PHP_XDEBUG=false

#
# For a full list of accepted values, see https://xdebug.org/docs/all_settings#mode.
##

-LOCAL_PHP_XDEBUG_MODE=develop,debug
+LOCAL_PHP_XDEBUG_MODE=profile

# Whether or not to enable Memcached.
LOCAL_PHP_MEMCACHED=false

@@ -54,10 +54,10 @@ LOCAL_DB_TYPE=mysql

LOCAL_DB_VERSION=5.7


# The debug settings to add to wp-config.php.

-LOCAL_WP_DEBUG=true
-LOCAL_WP_DEBUG_LOG=true
-LOCAL_WP_DEBUG_DISPLAY=true
-LOCAL_SCRIPT_DEBUG=true
+LOCAL_WP_DEBUG=false
+LOCAL_WP_DEBUG_LOG=false
+LOCAL_WP_DEBUG_DISPLAY=false
+LOCAL_SCRIPT_DEBUG=false

LOCAL_WP_ENVIRONMENT_TYPE=local


# The URL to use when running e2e tests.

diff --git a/docker-compose.yml b/docker-compose.yml
index 739c65f4a8..b781c3fe94 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -39,6 +39,7 @@ services:

environment:

  • LOCAL_PHP_XDEBUG=${LOCAL_PHP_XDEBUG-false}
  • XDEBUG_MODE=${LOCAL_PHP_XDEBUG_MODE-develop,debug}

+ - XDEBUG_CONFIG=profiler_output_name=cachegrind.out.%R

  • LOCAL_PHP_MEMCACHED=${LOCAL_PHP_MEMCACHED-false}
  • PHP_FPM_UID=${PHP_FPM_UID-1000}
  • PHP_FPM_GID=${PHP_FPM_GID-1000}

}}}

  • Install the deps: npm install && npm run build:dev.
  • Start the environment: npm run env:start && npm run env:install.
  • Open a post in the post editor. The URL would be something like this http://localhost:10035/wp-admin/post.php?post=492&action=edit
  • Inspect the generated cachegrind files, find the one relevant to the post editor (look at the URL), and copy it to your machine:

{{{sh
docker exec -it docker ps -f name=wordpress-develop_php -q ls -lt --full-time /tmp # List the files and inspect.
docker cp docker ps -f name=wordpress-develop_php -q:'/tmp/cachegrind.out._wp-admin_post_php_post=1_action=edit.gz' ~/<path-in-your-machine>
}}}

Repeat this process for trunk and this branch to compare the profiles. I'm sharing here the ones I've extracted in case anyone wants to just visualize them: trunk.gz (at 0f28f4cf1a38291664c238b6143fe68931787997), pr.gz

| | Trunk | This branch |
| --- | ----- | ----------- |
| wp_enqueue_registered_block_scripts_and_styles | Called twice and took 0.16ms. | Called once and took 0.15ms. |

By looking at this function's implementation, I've seen that the number of times wp_enqueue_style is called goes from 18 to 15. Digging a bit into the code, I haven't seen any filter/action that is relevant to have a big impact on performance.

I did notice the total execution time of the request for trunk was 951ms vs 862ms in the branch. While noticeable, I couldn't track the improvement to this change. I attribute the difference to factors other than this change (variability of the profiling, etc.).

Note: See TracTickets for help on using tickets.