Asynchronously load tinyMCE the first time a classic block is edited #21684
Conversation
This component allows us to wrap existing components and delay them until required 3rd party scripts and styles are asynchronously loaded. It also provides a hook for post-load setup that resolves before the children are rendered, guaranteeing that the children aren't active until the world around them is set up correctly.
| echo "<!-- Skipping TinyMCE in favor of loading async -->"; | ||
| } | ||
| } | ||
| remove_action( 'print_tinymce_scripts', 'wp_print_tinymce_scripts' ); |
There was a problem hiding this comment.
This action needs to be added in core, example PR here: WordPress/wordpress-develop#232
| <LazyLoad | ||
| scripts={ [ 'wp-tinymce' ] } | ||
| styles={ [ 'wp-tinymce' ] } | ||
| onLoaded={ () => window.wpMceTranslation() } |
There was a problem hiding this comment.
I'm not actually sure whether this would be how we do it. Given we can fully control what gets printed when Gutenberg is loading through the print_tinymce_scripts we can 100% wrap wp_mce_translation's result in a function to run in onLoaded... but it's also been floated that along with the new REST API that will provide the TinyMCE script, we could return the info necessary for initializing the library's i18n as a JSON blob.
Right now I think I prefer just printing this wrapped in a function in the action, but curious what the trade offs people can think of would be.
Co-Authored-By: Steven Dufresne <[email protected]>
…ation We were duplicating WP_Scripts and WP_Styles initialization and therefore misrepresnting what was handles were actually accessible through this API. By using the object, which for both these classes is accessed through a getter for the static global for each WP_Dependencies subclass, we can return the handles that are actually available. This makes it so that any script added by other plugins will also appear, for example. This doesn't change the accessibility of those handles, they were always already accessible. Something to consider is whether we should implement a blacklist, if there are scripts/styles that must never be requested in this way, as in, if they were then we're doing something really wrong. I'm not sure if any scripts like that exist today.
Rather than using `load-scripts.php` which only loads a small
subset of dependencies into a locally managed WP_Scripts/WP_Styles,
we can use the two new REST endpoints that use the globals
available throughout the rest of WP.
Each endpoint returns the full list of the URIs for the requested
dependencies as well as their respective dependencies. When multiple
dependencies are passed as in this format:
/wp/v2/scripts/?dependency=dep_1&dependency=dep_2
The endpoint will return:
[
{
handle: 'dep_1-dependency',
src: '...',
},
{
handle: 'dep_1-actual',
src: '...'
},
{
handle: 'dep_2-dependency',
src: '...',
},
{
handle: 'dep_2-actual',
src: '...',
}
]
That is, it will return descriptions for the list of
dependencies you pass to the endpoint, as well as those
dependencies dependencies, in the correct order in which
they would need to be loaded.
That means we can consume the response and just create a
script or link element for each response to be able to
load the dependencies we've requested.
We'll also store the actually loaded dependency handles
rather than the ones requested through the props. The thought
here being that one component might have a dependency
which itself depends on the dependency of another component.
Using the example response above, if component Foo depends on
`dep_1-dependency` and Bar depends on `dep_1-actual`, if Bar
is rendered first, then Foo does not need to have its dependencies
asynchronously loaded as they were already loaded as part of
loading Bar's dependencies.
At this point, this code doesn't actually fully work for
the classic block. Something is still broken and I'm still
trying to work out what exactly it is. However, the scripts
and styles do indeed load and the initialization is successful.
I _think_ there might just be a problem where something isn't
being correctly awaited, but I've run out of steam today to
figure out just what's going on.
| } else { | ||
| echo "<!-- Skipping TinyMCE in favor of loading async -->"; | ||
| echo "<script type='text/javascript'>\n" . | ||
| "window.wpMceTranslation = function() {\n" . |
There was a problem hiding this comment.
Shouldn't it be part of the REST API response in this async scenario?
I don't think it can be as simple as that for the block editor. Plugins still can register Meta Boxes that use TinyMCE heavily. AFC is a great example to test with.
There was a problem hiding this comment.
Yeah. Definitely an open question to me... I guess we can create a filter? The issue is the endpoint returns just the URI for the dependency, not the dependency script itself, so we can’t just concatenate the translation bit to it.
An alternative to this is to just have a tinymce-i18n endpoint to just return the json needed, the async onLoaded prop is suitable for that.
See inline comment for explanation for why this is necessary. Also refactors the dependency loading code into a single function. All of the logic is shared except for the actual DOM element creation. While the component was pretty simple to understand on one read through when there was no need for blocking, it didn't make sense to repeat that complexity and the comment for it. This also lets the component hide the complexity of loadScripts and loadStyles from the exported function, as well as the already-loaded- dependency tracking inside those closures.
Uses the "private" _WP_Editors class to retrieve the TinyMCE translations JSON blob and requisite metadata for initializing the editor on the frontend. If we decide to follow this approach, we'll want to refactor the methods used from _WP_Editors into free functions, or else move them into a separate class that is only used for getting the TinyMCE translations. One marked improvement we could achieve is to avoid json_encoding the translations only to have the JSON.stringified on the client. We should just send them down as an object rather than an encoded string. I didn't do that for this commit because it's going to require a wee bit of refactoring in wp_mce_translation to get the raw translation object back rather than a string. That refactoring should just go along with the refactor that pulls stuff out of _WP_Editors.
| onLoaded={ async () => { | ||
| const { | ||
| translations, | ||
| locale, | ||
| locale_script_handle: localeScriptHandle, | ||
| } = await apiFetch( { path: '/wp/v2/tinymce-i18n' } ); | ||
|
|
||
| const { tinymce } = window; | ||
| tinymce.addI18n( locale, JSON.stringify( translations ) ); | ||
| tinymce.ScriptLoader.markDone( localeScriptHandle ); | ||
| } } |
There was a problem hiding this comment.
@gziolo I think these changes will be interesting to you 😊
cc @griffbrad because we'd also discussed using an endpoint to retrieve the translation json.
| * | ||
| * @see WP_REST_Controller | ||
| */ | ||
| class WP_REST_TinyMCE_I18n_Controller extends WP_REST_Controller { |
There was a problem hiding this comment.
Still POCing, needs tests and an actually correct schema!
|
I'm probably missing something (or a lot of things) but... Why is all the REST API stuff needed here? What is the purpose of it? Is it just to get a script's URL? Why would it need to use REST and then load the script(s) or stylesheet(s) without using REST? How is that better than loading them directly and not using REST at all? If the current Script Loader in core needs to be used, it would be pretty simple to add a function that will get a script's handle as param, run wp, calculate the dependencies and output either the script tags plus the inline scripts, or the (concatenated) JS or CSS as a blob, or... anything else that makes sense. In TinyMCE's case using REST imho seems even more "not-useful" as it doesn't have any dependencies and generally cannot be used as dependency. Getting the URL to |
|
I definitely share @azaozz's concern (and his missing of things 😊) that the Theoretically it may feel nice to reduce With that being said, this PR is great because the |
|
@azaozz @StevenDufresne If this was only ever intended to support TinyMCE, it might not be necessary to go to the extent of implementing a set of REST API endpoints. But assuming we'd want to apply this pattern to other blocks, or to other parts of the editor screen, I sense it's not something we can prepare ahead of time during the initial load. At least, I would think the full dependency tree of all scripts would be quite large and not something to send over the wire (worth verifying). Instead, I think we'd need to be able to compute on-the-fly if and when a script is needed to be loaded. As I see it, the endpoint largely encompasses what you've described as a function receiving a script handle, just that it's exposed via the REST API. To the worry about delays from network requests: For the ones we can know ahead of time, there's already a pattern to preload REST API results at load time, so that the editor never needs to actually send a network request ( |
|
Yes, to reiterate what @aduth is saying here and in #21738 (where I also wrote some more general comments about this being a generic approach to a specific solution... because we had to start somewhere and TinyMCE is a big win if we can get it), I don't want us to get bogged down in the weeds of TinyMCE's particularities. RE: whether TinyMCE is too complicated a starting point for this... I feel strongly like I must be missing something here given who all is skeptical of this... but the draft PR does indeed work, you can try it out if you run WP with WordPress/wordpress-develop#232 to add the action that the Gutenberg PR uses to prevent TinyMCE from loading at the start. TinyMCE will not be loaded, but the second you edit a classic block, you'll see TinyMCE start to load and eventually the block works. I'll add more specific testing instructions to the PR to make it clear that this is a working POC. RE: @azaozz concern about why a REST API is necessary: I'm in total agreement with @StevenDufresne here... the REST API helps out with the block directory is a non-trivial way. Below I propose a hybridzed approach based on @aduth's suggestion at the end of this comment #21738 (comment). Hopefully this will help explicate why we do need a REST API while also addressing your concern about spinning up WP for too many REST requests. For now if only the classic block uses this, I don't think the concern about excessive requests applies yet, and below I've described a way we can proactively mitigate that problem by basically doing as you've suggested @azaozz and using preloaded URIs. By tracking the already loaded dependencies on the frontend inside a single component/object, we avoid having to pass around a list of the already loaded handles, which @azaozz mentioned was a big concern in the original trac ticket discussing async script loading. How I see it is that we don't need to have something that is expressly a concern of the client's state leak into WP... I think we can completely separate these concerns between the server and the client. @aduth that's great that there's a way to preload that information to make it accessible without needing to make another request. Totally understand the problems here. I think that given there's a need for this in the block directory, I wonder if we can do a hybridized approach. @aduth mentioned using the block registration API for this. That sounds great to me, it's totally possible for I think that this doesn't actually add a whole lot of complexity to the Because TinyMCE is such a big win (@griffbrad puts it a nice way... that TinyMCE is the equivalent of react, react-dom, and loads more) I'd like to propose the following:
|
Right. The question is "how" to do this in a good, future-proof way, not whether to use REST or not. As far as I see there are two general directions:
A third, imho best, option would be to refactor/re-think/redo WPs Script Loader and bring it from 2005 to 2020 :) Add support for defer, async, type=module, do HTTP/2 (non-concatenated) loading, etc. I know, this is way out-of-scope here but will need doing sooner or later, and adding something "temporary" now just pushes it back. In short, thinking that #21244 and https://core.trac.wordpress.org/ticket/48654 are not quite ready yet and perhaps there is an opportunity to make this work better. As for loading TinyMCE's scripts on demand, perhaps outputting the needed URLs on initial page load would be sufficient for now. The same is probably true for outputting the translation strings, they aren't that big. |
As far as I see the actual dependency data is not that big. The "before" and "after" add quite a bit to it but not sure they would be needed/wanted initially. However don't think we'd need to pre-load that data as we can get "chunks" of it through REST at any time, and just subtract the already loaded scripts. |
Document types, props, functions, and use cases for the LazyLoad component. Also adds WPNode to @wordpress/elements to allow for the correct type annotation for a ReactNode prop (like children).
|
@aduth @saramarcondes Great stuff! I'll also admit that I overstated the HTTP request issues to draw out some of the mitigating strategies we have in mind. There are plenty of ways we will be able to create a progressive experience and this work is definitely the precursor, but in my opinion having some of those strategies on the board will be more beneficial than distracting.
Good call. Here's some info on the Block Directory:It currently works like this:
A better flow would be:
The |
|
In the related issue, @gziolo mentioned that opening up an option for the classic block to be in block directory could potentially get the async loading capabilities. However, that would only solve the issue for when the classic block is not already used and would cause site owners to manage this preference. I think the solution that I have here where we create a way for blocks to load their dependencies even if they actually exist within a post is better, for a couple reasons:
Ultimately, I think this boils down to this: the block directory already asynchronously loads dependencies for the blocks that support it, but once you have installed a block, you're now paying the penalty for it every time you load your editor. This means that blocks have a hidden cost that isn't obvious and would be impossible to communicate. Ultimately, that means that block developers get saddled with trying to make harder performance decisions than are necessary, if they're performance minded at all. Moving towards a solution where we could get all blocks with dependencies to asynchronously load their dependencies means that we remove that hidden cost. It also opens us up for nice performance wins for core blocks with big dependencies like the classic block. In fact, that's the best place for us to start because we can experiment with a heavy use case to see what approaches are long-term viable. @azaozz and @ellatrix, something @gziolo mentioned is that there may be some hidden dependency on TinyMCE in the editor that I haven't noticed yet. @gziolo said that y'all two would be the best to ask about this. When I search through the Gutenberg codebase, the only references to |
|
Closing in favor of #23068 which is a more focused PR for strictly addressing async TinyMCE without any of the extra frills. |
Description
Added
LazyLoadcomponent. This component allows us to wrap existing components and delay them until required 3rd party scripts and styles are asynchronously loaded. It also provides a hook for post-load setup that resolves before the children are rendered, guaranteeing that the children aren't active until the world around them is set up correctly.I've then wrapped
ClassicEditinLazyLoad. This 100% does not work and is just a proof of concept.A related PR: #21244
We'd replace
load-scripts/stylesin the current code here with requests to those endpoints.Types of changes
New feature
Checklist:
Testing instructions
wordpress-developby following the instructions forwp-envconfiguration here: https://github.com/WordPress/gutenberg/blob/master/packages/env/README.md#wp-envjson. Run WordPress usingnpx wp-env startinside your Gutenberg repo.npm run devwindow.tinymce === undefinedwindow.tinymceis defined.