Render block preview on the server #55850
Conversation
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/class-gutenberg-render-blocks-controller.php ❔ lib/compat/wordpress-6.5/rest-api.php ❔ lib/load.php |
|
Size Change: +3.9 kB (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a4e7b28. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7126692771
|
draganescu
requested review from
youknowriad,
scruffian,
jsnajdr and
ellatrix
draganescu
added
the
[Feature] Patterns
| @@ -18,6 +18,9 @@ import { | |||
| import { useInstanceId } from '@wordpress/compose'; | |||
| import { __ } from '@wordpress/i18n'; | |||
| import { Icon, symbol } from '@wordpress/icons'; | |||
| // ignore restricted import ESLint error for apiFetch | |||
| // eslint-disable-next-line no-restricted-imports | |||
| import apiFetch from '@wordpress/api-fetch'; | |||
There was a problem hiding this comment.
The block editor can't depend on apiFetch as it's a generic JS package. What we should do here is to provide a block editor setting that is called renderPreview or something that is basically a component that takes a list of blocks to render their preview. And if this setting is not provided, we should keep the existing preview behavior (third-party editors...)
There was a problem hiding this comment.
Yes I've commented as well that this is a known issue. The eslint disable is for demo purposes.
If the endpoint gets a list of patterns and returns a list of rendered html then the patterns that are passed to this component list can include their rendered html and this fetch would go away. Why would we need an editor setting?
There was a problem hiding this comment.
This component is about rendering previews regardless of whether they come from pattern, the example of a block type, variations or any random block list. Adding the rendered html to patterns won't solve all the use-cases.
There was a problem hiding this comment.
The block preview component optionally gets the HTML. It is being passed this HTML depending on the context. In this case only it comes from block patterns list which gets it from whomever wants to render a pattern list. So yes, what I am saying is I only want to solve the pattern preview case but the low level block preview will be ready for future situations. I don't understand what the editor setting would be for, it's probably obvious :D
There was a problem hiding this comment.
Why would a BlockPreview component get HTML, it's not block preview anymore, it's just an iframe in that case and there's no need to use this component.
For me, we should solve all the use-cases at once because they're just as problematic in terms of performance.
I don't understand what the editor setting would be for, it's probably obvious :D
This is some pseudo code, to explain the idea.
const BlockPreview = ( blocks ) => {
const html = useSelect( select => select( 'core/data' ).getHtmlForBlocks( blocks ), [ blocks ] );
return <Iframe>{html ?? <Spinner />}</Iframe>
}
<BlockEditorProvider settings={ ...settings, BlockPreview } />and the BlockPreview component could do something like:
const { BlockPreview } = useSelect( select => select( 'core/block-editor' ).getSettings().BlockPreview, [] );
return BlockPreview ? <BlockPreview blocks={blocks} /> : <CurrentBlockPreviewCode blocks={blocks} />There was a problem hiding this comment.
I'm not against two components to render the previous, but in that case, It's not even a StaticBlockPreview it's just Iframe
There was a problem hiding this comment.
I think there's still some hot-wiring to make an <Iframe> <StaticBlockPreview>. The injected styles and scaled size as mentioned by @draganescu for instance, but also like pointer-events: none and other stuff that only makes sense in the "preview" context. We probably will end up abstracting them into a different component, but the name should be more explicit than <Iframe>.
There was a problem hiding this comment.
Yes, as @kevin940726 describes the helper components that block preview needs will have to be abstracted and provided by a 3rd package if we want to give editor implementations the ability to implement their own previewer - like this PR demonstrates for the site editor.
I don't think we should modify the patterns API - previewing is very special - again as this PR demonstrates - in the sense that we need to build some context for the rendering to be as close to a real preview as possible - which complicates the process enough to warrant a special endpoint. I also agree with @youknowriad that if should have a blanket solution for all previews: patterns, blocks, block styles.
There was a problem hiding this comment.
previewing is very special - again as this PR demonstrates - in the sense that we need to build some context for the rendering to be as close to a real preview as possible - which complicates the process enough to warrant a special endpoint.
Could you elaborate what specifically is the special handling we need to do in a separate API request, as opposed to maybe a PHP function call instead? There's nothing stopping us from providing both the endpoint and patching the patterns API too. I guess the main concern I have here is that a round-trip time for each preview depends on network condition and might just be slower for some users (high-end device but with slow internet). Not to mention that we're not doing batching and we'll soon reach the cap of maximum concurrent requests the browser can make at a time. We're also not doing error management and retrying which make this a little bit more complicated. I think it'll be easier if we can just move them all to the patterns API so that it can also be cached correctly too (any reason we're using POST for this endpoint that can't be cached by the browser?).
There was a problem hiding this comment.
I think your observations are correct. There's nothing stopping us from providing both the endpoint and patching the patterns API too, indeed.
what specifically is the special handling we need to do in a separate API request, as opposed to maybe a PHP function call instead
It's not specific handling it's specific logic that we require to execute to create a server context for rendering properly. We could have shared procedures (a function) between patterns and other custom endpoints, sure.
any reason we're using POST for this endpoint
We send this big blob of blocks. If this were part of the GET patterns this would also not be required.
| __html: html, | ||
| } } | ||
| ></div> | ||
| { /* <MemoizedBlockList renderAppender={ false } /> */ } |
There was a problem hiding this comment.
Some loader is probably needed while fetching the preview.
There was a problem hiding this comment.
ideally it should not be needed at this point - the html should exist via the props this component gets.
There was a problem hiding this comment.
not necessarily saying but the preview function should be async, whenever that thing is in flight, we should probably show some kind of visual indicator.
There was a problem hiding this comment.
I've refactored this to match the architecture suggestion from @youknowriad.
I think the path is good but we have some challenges:
rendering the shape of block list that the editor works with is unclear how to on the server-serializeto the rescue!- there is considerable duplication between the editor implementation and the abstract block editor in terms of how the preview works
...edit-site/src/components/block-editor/block-editor-provider/default-block-editor-provider.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is duplicated from the block editor package,
draganescu
changed the title
| } ); | ||
| } | ||
|
|
||
| function Iframe( { |
There was a problem hiding this comment.
This component is exposed from block-editor, why did you have to reimplement it?
| > | ||
| <EditorStyles styles={ editorStyles } /> | ||
| { contentResizeListener } | ||
| <div |
There was a problem hiding this comment.
Is this div the only thing that is different compared to the default block preview? If that's the case, why not just make the blockPreview setting return just this part and not the whole thing and just reuse the BlockPreview except this part.
There was a problem hiding this comment.
I'll review this - I think initially I thought there would be more simplification. I'll see if there is more if not I'll go with your observations. 🙏
draganescu
force-pushed
the
try/prerender-patterns
branch
2 times, most recently
from
da1558b
to
34967f0
Compare
|
I've hit a snag here: some patterns include image blocks with no image, and they should show a placeholder. But when rendering these blocks on the server we do not show placeholders, instead we show nothing. So previews for patterns or other things that are supposed to show placeholders show up empty. One way I can think of is to set a context in the preview render endpoint that tells the image block to render the placeholder. I'll look into this path. |
Maybe it was a mistake to start using this "if certain attributes are not filled in, render nothing at all" logic in the first place. I've been recently very surprised by a similar behavior with a Social Icon block that doesn't have an URL set. You create some site layout, planning to fill details later, and decide to preview it, and some parts are outright missing. |
draganescu
force-pushed
the
try/prerender-patterns
branch
from
9913831
to
fa50250
Compare
draganescu
force-pushed
the
try/prerender-patterns
branch
from
bd758b4
to
a4e7b28
Compare
|
The snag is more complicated than it appears. The snag is that blocks who are set empty will render whitespace on the server, but will render placeholders or examples in the editor. That means that server preview is an inferior experience, even a broken one sometimes (e.g. a TT4 pattern that has only image blocks in it on two colums, renders nothing on the server for preview, it looks broken). I thought it's be something approachable but it's not so easy. To illustrate, one of the things we came up with was to use the example data to render previews. Sounds like a decent idea, most core blocks if not all have examples so we could use that. While true that most examples are in the client side init, it's easy to move the example to Case in point the image block:
So this is still stuck at: how to preview empty blocks on the server in a way in which they show up something, an example or a placeholder, without having to implement a custom solution for every block? |
|
Have the recent performance improvements made any noticeable difference? It should speed up everything inside BlockList. |
@ellatrix you can test this. To me on on a fast computer TT4 trying to swap template using the menu in the template editor's inspector, template tab, dot menu, replace template, it is still noticeably laggy and stutters. I think no matter how much we optimise the performance of the editor, instancing an inline editor per item previewed will be problematic - maybe a setup will show 98 patterns in that modal, what is the upper limit our optimisations still underperform? Also rendering serverside, and fixing server side rendering of previews, opens up new possibilities around themes providing nice demo content that can be easily overriden without polluting the templates with hardcoded content, so there is side value in that too if this PR lands. However, to advance here, we need to do at least a couple of things, if I am not mistaken:
If we can achieve 1 and 2 we can then easily come up with a rendering "context" which renders blocks using example data on the server, |
scruffian
force-pushed
the
try/prerender-patterns
branch
from
a4e7b28
to
23f3b41
Compare
FWIW, performance is only one aspect of the story here. An editor also uses a massive amount of memory to power all the editing experience. I think either way rendering previews statically can still have lots of potential! |
| setHTML( dataHTML ); | ||
| }; | ||
| getHTML().catch( ( error ) => { | ||
| return error; |
There was a problem hiding this comment.
What's happening with this error? It seems to be swallowed right?
Should we instead set some local error state (as you do with the setHTML) and then use this to display an error message instead of the preview? Or you could revert to rendering client side.
draganescu
requested review from
artemiomorales,
michalczaplinski and
ajitbohra
as code owners
| // Updates all blocks to use their example data, if they have it. | ||
| function modify_block_attributes_before_render( $block ) { |
There was a problem hiding this comment.
I think we'll need to explain why this is being done 🙏
Co-authored-by: Dave Smith <getdavemail@gmail.com>
draganescu
requested review from
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
draganescu
force-pushed
the
try/prerender-patterns
branch
from
136e850
to
35e5dd1
Compare
draganescu
force-pushed
the
try/prerender-patterns
branch
from
35e5dd1
to
c177616
Compare
|
In this commit I exemplify with bad code the idea of always using values from attributes when rendering dynamicly. This is a convention based system. Since the dynamic render function allows the developer to write the rendering in any way possible, we can't enforce a system in which the data pipeline is fixed. So we need a sort of convention which, if you follow, you get benefits. In this case if the block binding API, and hence partially synced patterns and block previews, work on the convention that the data always comes from attributes, it's a good incentive to embrace the convention. In c177616:
This way we don't have to do any HTML acrobatics. We only need to override the attribute. I think this should be attempted, the idea that to change rendered value at the system level only attribute data should be updated, not markup. Markup acrobatics should be reserved for on the spot, block specific handlers, which should not be managed by system level featured (block bindings, pattern overrides/synced patterns or block preview). Extra notes:
|
|
I tried moving the basic feature to a different PR here: #57977 |
|
Close in favor of #57977 - this other one is a more streamlined implementation. |
Attempt to fix #54999
This PR registers a custom endpoint that renders a block tree and returns the html. This endpoint is used in the
BlockPatterncomponent (ofBlockPatternList) which usesBlockPreview.BlockPreviewis augmented to receive a newhtmlprop which is passed down toScaledBlockPreview(fromAutoBlockPreview) and set to be thedangerouslySetInnerHTMLof a div in the preview iframe.This oprimisation makes all the instances of
BlockPatternListbe much snappier: the new page pattern selector, the pattern inserter or the template swap modal.Known issues
Known limitations
some blocks render a placeholder on when used in the editor in an undefined context. For example the "Archive Title" block shows a "Tag: Name" placeholder when one creates a tag index template. When this block is rendered on the server it will need a global context to render a tag or it renders nothing.
In order to avoid having to update blocks, the solution here is to provide a global context for every context we preview in. We can "detect" the context in the editor and send it to the REST API endpoint. For instance, when previewing patterns in a template in the site editor we can send the template name. We can then build a WP_Query and a Post based on the most recently published content.
To do:
the markup should come with the pattern list instead of being requested per pattern, and so drop the need to use
api-fetchin the block editor packagethe api endpoint should get a list of patterns, that is be specific to this not render generic blocksrendering blocks on the server require that some of the global context never be undefined. We need to update render_block to always have the global context keys set, even if they are null.
Screen recording
pre-rendered-patterns.mp4