SSR: Fix directive processor invocation#215
SSR: Fix directive processor invocation#215ockham wants to merge 2 commits intomain-wp-directives-pluginfrom
Conversation
wp-directives.php
Outdated
| block_has_support( | ||
| $parent_block->block_type, | ||
| array( | ||
| 'interactivity', | ||
| 'isolated', | ||
| ) | ||
| ) | ||
| isset( $parent_block ) | ||
| ) { | ||
| $parsed_block['isolated'] = true; | ||
| if ( | ||
| block_has_support( | ||
| $parent_block->block_type, | ||
| array( | ||
| 'interactivity', | ||
| 'isolated', | ||
| ) | ||
| ) | ||
| ) { | ||
| $parsed_block['isolated'] = true; | ||
| } | ||
| $parsed_block['inner_block'] = true; // TODO: Limit to interactive blocks. |
There was a problem hiding this comment.
I'm kind of piggy-backing on an existing, similar mechanism here (set a block attribute during render_block_data; inspect presence and truthiness of that attribute during render_block to act accordingly). h/t @luisherranz and @dmsnell for the prior art and recommendation.
I wish we could only use one attribute and move some logic into the functions that hook into render_block, but since the isolated attribute is set depending on the presence of block support for the current block's parent, that might be impossible, since we don't have access to the parent inside of render_block AFAIK.
f2f94d8 to
9ccf375
Compare
|
I was just testing this by merging it to the It seems that the HTML from the server is correct, but it gets malformed after hydrating. I made a quick video trying to explain it better: Basically, for this HTML: <div data-wp-context=‘{ “isOpen”: false }’>
<div data-wp-show=‘context.isOpen’><span>WP Show</span></div>
</div>The HTML from the server is correct: <div data-wp-context=“{ “isOpen”: false }“>
<template data-wp-show=“context.isOpen”>
<div><span>WP Show</span></div>
</template>
</div>In the client, an extra empty <div data-wp-context=“{ “isOpen”: false }“>
<template data-wp-show=“context.isOpen”>
<div><span>WP Show</span></div>
<template></template>
</template>
</div>In the video, I'm not using the latest version of the branch, but I tested it again with the latest changes and the problem is still there for me. |
|
Thank you Mario for testing! 🙌 I’m a bit relieved that the issue is on the client side only, and that it doesn’t occur when you disable JS, as that confirms that it must be an issue with our client-side code (and not e.g. malformed HTML that the browser "fixes" when building the DOM). |
|
I made a Loom to explain the issue: |
|
Hmm, I think I've found another instance of wrapped <template data-wp-show="selectors.wpmovies.isPlaying"><template ><div class="wpmovies-video-player wp-block-wpmovies-video-player">
<div class="wpmovies-video-wrapper">
<div class="wpmovies-video-close">
<button class="close-button" data-wp-on.click="actions.wpmovies.closeVideo">
Close </button>
</div>
<iframe src=""
width="420"
height="315"
allow="autoplay"
allowfullscreen
data-wp-bind.src="state.wpmovies.currentVideo"
></iframe>
</div>
</div></template></template>(This is again the Movies Demo when used locally with the |
|
Oh, I just remembered that the Maybe that's the reason why it is not working as expected in the video I shared previously.
I confirm that I can also replicate the issue you reported in the movies demo 😢 Apart from that, I have one question: is it correct to move the |
Ah yeah, that seems likely!
TBH I was really just trying to follow what seemed to be the outcome of the discussion you mentioned at the time:
Should I leave the |
So there's a small problem with templates and vdoms that I haven't figured it out completely yet. https://www.loom.com/share/a8fe41eeb30343cc8fc41f08d0045b41 At first glance, it looked like moving the |
|
Oh, wait. I just remembered that I actually did some coding for this. This is <!-- This was SSRed as false in the server -->
<template data-wp-show="context.showImage">
<img src="..." />
</template>
<!-- This was SSRed as true in the server -->
<img data-wp-show="context.showImage" src="..." />In this implementation, the HTML is removed from the client when What I'm doing here is saving the HTML of the template in a separate prop: const isTemplate = localName === "template";
const childNodes = isTemplate ? node.content.childNodes : node.childNodes;
// ...
if (isTemplate) return h(localName, { ...props, templateChildren: children });That way, the hydration doesn't add the inner contents of the template to the DOM. Then, in the const children = useMemo(
() =>
element.type === "template" ? element.props.templateChildren : element,
[]
);And finally, I remove the HTML from the DOM in the client when if (!evaluate(show, { context: contextValue })) return null;
return children; |
|
Since this PR isn't quite fixing all the issues we were seeing, and we're looking to fix what seems to be an underlying issue with the Tag Processor, I'm considering closing this PR. There's still the separate issue of processing directives repeatedly (once for each layer of nested inner block) which is fixed by one of the commits in this PR (a8cadac), but I'm thinking to file a new PR for that to keep that conversation separate. |
There are currently two problems with the way we invoke our directive SSR processors:
wp_process_directivesintorender_block, it gets run repeatedly on markup inside nested inner blocks (once for each level of block nesting around a given piece of markup).next_tag(), without flushing the changes made to the current tag.Any issues arising from these remained undiscovered until #141, which introduces the
data-wp-showdirective. The latter is a bit special in that it wraps a tag with said directive in a<template>tag (if the directive attribute value is falsy), and moves thedata-wp-showdirective to that newly prepended<template>).Instructions to reproduce the issue(s)
The following issues were observed when running the
wp-movies-demorepo locally in connection with a local copy of theblock-interactivity-experimentsrepo, with theadd/wp-show-ssrbranch (from #141) checked out.To do the latter, you can create a file named `.wp-env.override.json` in the `wp-movies-demo` root with these contents
{ "plugins": [ "https://downloads.wordpress.org/plugin/gutenberg.latest-stable.zip", "https://downloads.wordpress.org/plugin/wordpress-importer.latest-stable.zip", "../block-interactivity-experiments", "." ] }Because of the aforementioned issues, we would then encounter markup such as #141 (comment):
Note the three nested
<template>s -- two of them are due to the fact that the block that contains thedata-wp-showdirective is nested in another block. Finally, the outermost<template>is due to the fact that another directive (data-wp-classis present and processed beforedata-wp-show, without flushing (i.e. callingget_updated_html()) prior to invoking thedata-wp-showdirective processor.This PR aims to fix both issues. To verify, follow the above instructions to reproduce the issue.
Then, on top of the
add/wp-show-ssrbranch, apply the changes from this branch (e.g. by merging this branch into it, or by cherry-picking the commits), and test again. The markup should now be fixed:Automated test coverage, or the lack thereof
It's unfortunately a bit hard to add proper unit test coverage for this -- arguable as it's more of an integration problem (that would require integration tests).
In the process of investigating this issue, I added some tests to #141 to reproduce issue number 2 (e.g.), but they can't really count as unit tests: While they helped detecting the issue, we cannot make them pass by applying the code changes from this PR; instead, we'd need to add
get_updated_html()calls right inside test (afteradd_class, beforenext_tag). Similarly, verifying that a function called from therender_blockhook runs only once (rather than repeatedly) for nested blocks isn't really unit test material.Arguably, this PR fixes #148.