#45065 closed enhancement (fixed)
Include Gutenberg packages in WordPress core.
Reported by: | omarreiss | Owned by: | 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)
Change History (56)
This ticket was mentioned in Slack in #core-js by atimmer. View the logs.
6 years ago
#5
@
6 years ago
45065-wip.diff is a working patch that builds all available necessary Gutenberg packages from npm.
#6
@
6 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.
6 years ago
#8
@
6 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.
#10
@
6 years ago
FYI - This seems to have broken Travis: https://wordpress.slack.com/archives/C02RQBWTW/p1539252383000100
#12
@
6 years ago
Code reviews for 45065-script-loader.diff is located on GitHub: https://github.com/Yoast/wordpress-develop-mirror/pull/119
This ticket was mentioned in Slack in #core-privacy by azaozz. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-committers by pento. View the logs.
6 years ago
#17
@
6 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
@
6 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.
#20
@
6 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
@
6 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 :)
#25
@
6 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
@
6 years ago
- Keywords fixed-5.0 added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for merging to trunk.
#27
@
6 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
.
This ticket was mentioned in Slack in #core-js by omarreiss. View the logs.
6 years ago
This ticket was mentioned in PR #4356 on WordPress/wordpress-develop by @oandregal.
20 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:
20 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>
}}}
- Open the file with any application that visualizes valgrind-like files. For example, kcachegrind (qcachegrind for mac).
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.).
#44
@
20 months ago
Follow-up at https://core.trac.wordpress.org/ticket/58208
@oandregal commented on PR #4356:
20 months ago
#45
Committed at https://core.trac.wordpress.org/changeset/55695
In 43687: