Block Library: Implement Template Part block editing.

[epiqueras]

Depends on #18739
Follows #18736

Description

This PR continues the block template part work by enabling the editing of existing block template part files as blocks in block templates.

It runs the block template override logic in the editor to find the relevant block template and create auto drafts for both the block template and its nested block template parts. This is done in the editor settings hook, to be able to send back context to the editor, like the current template's ID.

The edit implementation of the block template part block can then load the block template part CPT post directly if the block template part has already been customized or it can look for the latest auto draft which was created from the block template part files.

How to test this?

• Set up the test environment described in Block Library: Add core template part block. #18736's testing instructions.
• Create a block template CPT post called "single" with /block-templates/single.html's contents.
• Verify that editing the header block template part and saving both the block template and the block template part propagate the changes everywhere the header is used.

Types of Changes

New Feature: The Template Part block now lets you edit the underlying block template part.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[jffng]

Hi @epiqueras, I'm attempting to follow along with the progress on block-based themes.

Create a block template CPT post called "single" with /block-templates/single.html's contents.

I'm able to create the block template and edit and save the template part successfully:

Verify that editing the header block template part and saving both the block template and the block template part propagate the changes everywhere the header is used.

However it does not propagate. Instead, it creates a new single template and untitled template-part which are not resolved by the template loader:

[epiqueras]

This PR continues the block template part work by enabling the editing of existing block template part files as blocks in block templates.

This PR only enables the editing of existing block template parts from files. E.g. #18736's /block-template-parts/header.html.

It looks like you are trying to create a new template part in the editor, which is not yet supported.

[aduth]

Is there not an equivalent selector that grants us access to the serialization of these blocks, rather than exposing the internal utility functions? Would it be possible to implement such a selector, or enhancement to an existing selector?

I'm not entirely clear what it is about the editor's serialization that we want to reuse here, vs. the raw @wordpress/blocks implementation, but it would be good if we can avoid exposing these internals, limit the use to either the standard public interface of a store (selectors) or the base implementation of wp.blocks.serialize.

[epiqueras]

There is no selector that just serializes the inner blocks of a given block.

We could extend getEditedPostContent, but the name wouldn't really do it justice and the editor store doesn't really work at the blocks level.

We need this util from the editor, for the backwards compatibility reasons explained in the file:

https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/store/utils/serialize-blocks.js

I think this utility could be moved to @wordpress/blocks as legacySerializeBlocks or something. Or maybe the backwards compatibility concerns are not an issue anymore? What do you suggest?

[aduth]

I think those behaviors might still be necessary for the post editor, but I don't know that they would need to extend to templates. The main purpose for the post editor is:

• Avoid saving a post which consists of just an empty paragraph (more a user-facing thing, since technically a new paragraph block is non-empty content, but for the purpose of a post is considered non-empty, to avoid accumulating drafts)
• Retain existing wpautop formatting applied to posts. This is relevant for content published before the block editor, or otherwise processed using the_content filter on the front-end.

I don't think either of these are really relevant for the template parts though?

To me, it seems like maybe we can keep that utility for default usage, then just use the block module's default serialize for this.

[epiqueras]

Perfect, we can just use serialize here then. I'll update it.

[aduth]

In the above useMemo, we test whether content is a function. Here, we have no such safety checks. Do we run any risk that this isn't a function?

Aside: I'm not really clear why this is a function.

[epiqueras]

useMemo runs on the first render. InnerBlocks will set it to a function after that.

Aside: I'm not really clear why this is a function.

It mirrors how the editor store works. It uses a function that computes the eventual value to make the entity dirty in the store without having to run a potentially expensive computation.

[aduth]

useMemo runs on the first render. InnerBlocks will set it to a function after that.

Is this behavior documented somewhere? Could we at least include an inline comment to explain this?

[epiqueras]

I'm refactoring this into a custom hook with docs, because it will be used often, useSyncedEntityInnerBlocks.

[aduth]

I would not expect this mapping function to have a side effect.

[epiqueras]

What's the problem with it?

[aduth]

What's the problem with it?

One of the major goals of a getter / setter pairing of useSelect / useDispatch is to provide clear signals to a developer where and how mutations are expected to occur. There would be less confidence in this holding true if a developer is expected to need to account for potential side effects occurring within useSelect. In more practical terms, a potential refactor of this component could result in these side effects being triggered more or less often than originally expected; maybe if memoization was altered or removed (as a developer should otherwise assume that a mapSelect callback will be a pure function).

For me, it's largely a matter of maintenance overhead and potential for bugs introduced as part of said maintenance. There's otherwise nothing technically incorrect about this implementation.

[epiqueras]

maybe if memoization was altered or removed

Good point, I'll change it.

[aduth]

These new props value and onChange (aside: also not documented) seem contrary to the purpose of InnerBlocks as an abstracted, managed rendering of blocks. For something like this where we want full control over the rendering of blocks, is there any reason not to render a custom BlockEditorProvider instead?

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/provider/README.md

Or, alternatively, based on what we're doing above with serializing blocks and how it's proposed to expose the internal state utilities, maybe this is something which should be baked into InnerBlocks. Is the idea that we need some way to customize the "save" or "change" behaviors, in this case to sync to the entity?

For that, I might be okay with an onChange, but more as a true side-effect, where we're not intending to fully control the value of InnerBlocks, but rather sync any changes.

Then again, that seems like something we could also achieve without a callback, and instead using the getBlocks selector to track changes to the inner blocks of this block.

To me, this is a very similar problem to what was previously discussed in #5596 around having a callback to setAttributes, where instead the model we encourage is where one should detect the changes using the existing selectors functionality (even easier now today with hooks than it was at the time of #5596).

[epiqueras]

is there any reason not to render a custom BlockEditorProvider instead?

We don't want a split block list. We want the child blocks to still save to the parent through the block's save component where it makes sense, and we want the UX of a single block list.

This change provides a way to provide an initial value and then sync with the changes of InnerBlocks so that other code can use the child blocks to do things like save to other entities.

We could achieve something similar with actions and selectors, albeit in a much more verbose and error-prone way. We would probably end up implementing a custom hook for it and the logic for checking if the last check was persistent might get messy. This approach would also be less performant.

I don't think this is comparable to a callback for setAttributes. In that scenario people needed a way to react to changes in the store, and that's what subscriptions are for. In this scenario we want to control/sync with the state of a component.

What are the issues you see this creating in the future?

[aduth]

This change provides a way to provide an initial value and then sync with the changes of InnerBlocks so that other code can use the child blocks to do things like save to other entities.

I think the issue for me is that we've now introduced (to me at least) some confusion around exactly what InnerBlocks is. Is it a controlled input? Or is it abstracted to maintain the value internally? Where before it was clearly the latter, with these changes it seems like the answer is now "both". A concern stems from the need to document that clearly, and to maintain that in perpetuity.

For me, this sort of controlled rendering was was BlockEditor should do. I understand the usability need that we need blocks within this get treated as part of the same "tree". It reminds me of similar work in #14715 with the reusable block, and where we want to draw this line of separate vs. integrated editors. Is there value in the reusability of block-editor if these limitations prevent us from using it in the editor? Is the purpose of these InnerBlocks revisions to serve as an enhanced version of a block editor? In the end, maybe this is the right change, since we could have a use case for each, but it is worth having the clear picture for how all these fit together.

Maybe if we compare it to React's distinction of controlled vs. uncontrolled inputs, it's fine to consider support for both use-cases. I think there's a bit of a difference in how what would become the equivalent of an "uncontrolled" InnerBlocks has some inherited behavior (and an inherited default value of an inner block) whereas, without a defaultValue, any other input would always be empty.

We could achieve something similar with actions and selectors, albeit in a much more verbose and error-prone way. We would probably end up implementing a custom hook for it and the logic for checking if the last check was persistent might get messy. This approach would also be less performant.

To a lesser degree, I still think we'll have some concern about how difficult it is to manage this value from a block. For example, with this implementation, are we serializing blocks on every keypress within a template? That seems like it could become a performance concern for non-trivial templates. Whether that's something we could improve, it would be fair to acknowledge that others may encounter similar pain points.

I'm not sure it's something we need to act on now, but it gets me me wondering whether there are alternative / additional abstractions we could imagine to help encapsulate this behavior of mapping the blocks data to a custom entity serialization. As before I was talking of this sort of InnerBlocks being an enhanced version of a BlockEditorProvider, I wonder if there's some parallel of how Editor is an enhanced version of BlockEditor responsible for managing how a block editor saves to a post.

[epiqueras]

I think the issue for me is that we've now introduced (to me at least) some confusion around exactly what InnerBlocks is. Is it a controlled input? Or is it abstracted to maintain the value internally? Where before it was clearly the latter, with these changes it seems like the answer is now "both". A concern stems from the need to document that clearly, and to maintain that in perpetuity.

That's fair. I agree we should avoid it if possible.

For me, this sort of controlled rendering was was BlockEditor should do. I understand the usability need that we need blocks within this get treated as part of the same "tree". It reminds me of similar work in #14715 with the reusable block, and where we want to draw this line of separate vs. integrated editors. Is there value in the reusability of block-editor if these limitations prevent us from using it in the editor? Is the purpose of these InnerBlocks revisions to serve as an enhanced version of a block editor? In the end, maybe this is the right change, since we could have a use case for each, but it is worth having the clear picture for how all these fit together.

BlockEditor is for building entire new editors. InnerBlocks is for managing subtrees in editors.

Maybe if we compare it to React's distinction of controlled vs. uncontrolled inputs, it's fine to consider support for both use-cases. I think there's a bit of a difference in how what would become the equivalent of an "uncontrolled" InnerBlocks has some inherited behavior (and an inherited default value of an inner block) whereas, without a defaultValue, any other input would always be empty.

Yes, that's what I based it on.

To a lesser degree, I still think we'll have some concern about how difficult it is to manage this value from a block. For example, with this implementation, are we serializing blocks on every keypress within a template? That seems like it could become a performance concern for non-trivial templates. Whether that's something we could improve, it would be fair to acknowledge that others may encounter similar pain points.

We're not serializing on every keypress, but I also see this becoming a common pattern and it would be good to deal with all the pain points in one place.

I'm not sure it's something we need to act on now, but it gets me me wondering whether there are alternative / additional abstractions we could imagine to help encapsulate this behavior of mapping the blocks data to a custom entity serialization. As before I was talking of this sort of InnerBlocks being an enhanced version of a BlockEditorProvider, I wonder if there's some parallel of how Editor is an enhanced version of BlockEditor responsible for managing how a block editor saves to a post.

We could have an enhanced InnerBlocks that lets you turn on the syncing with a prop, but that would make block-editor depend on core-data. I think a useSyncedEntityInnerBlocks hook will work here. I've thought a lot about this and have much better ideas for how to deal with all this in a better way, specially with the new changes to master. I'll close this PR and open a new one soon.