#45065 closed enhancement (fixed)
Include Gutenberg packages in WordPress core.
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | fixed-5.0 |
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 (51)
This ticket was mentioned in Slack in #core-js by atimmer. View the logs.
2 years ago
#5
@
2 years ago
45065-wip.diff is a working patch that builds all available necessary Gutenberg packages from npm.
#6
@
2 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.
2 years ago
#8
@
2 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
@
2 years ago
FYI - This seems to have broken Travis: https://wordpress.slack.com/archives/C02RQBWTW/p1539252383000100
#12
@
2 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.
2 years ago
This ticket was mentioned in Slack in #core-committers by pento. View the logs.
2 years ago
#17
@
2 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
@
2 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
@
2 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
@
2 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
@
2 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
@
2 years ago
- Keywords fixed-5.0 added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for merging to trunk.
#27
@
2 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
.
In 43687: