Block Library: Implement Template Part block editing 2.

[epiqueras]

Alternative to #18925
Follows #18736

Revisions

• Block Library: Implement Template Part block editing 2. #19203 (comment)
• Block Library: Implement Template Part block editing 2. #19203 (comment)

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 some of the block template override logic in the editor to find the relevant post and create auto drafts for the template parts in it. This is done in the editor settings hook, to be able to send back context to the editor, like the current template's ID, later on, when implementing editing modes.

The edit implementation of the 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.

With a way to create and/or load the CPT posts for the template parts, we just needed a way to sync entities with inner blocks. This will be needed for many blocks and the parsing and serialization dance will be the same for all of them so it made sense to introduce 2 new hooks a new hook to core-data for this:

const [ blocks, onInput, onChange ] = useEntityBlockEditor( kind, type, options );

For somewhat low-level manipulation.

useEntitySyncedInnerBlocks( kind, type, options )

For just syncing a block's inner blocks the same way the block editor is synced with the editor. This uses the former hook in its implementation and will be the preferred way to handle things in most blocks, like the Template Part block.

Additionally, it looks like the core-data functionality of evaluating optimized function edits before saving, was never implemented. I think we decided to back off from it until we decided to "really" support them, and never got around to it. This PR adds it, because unlike the editor which has a single canonical way of saving through savePost, entity synced inner blocks can be saved in many different ways/UIs, so it's important for this to happen at the framework level. We'll still need to keep the specific savePost behavior, because the editor uses a custom serialization function for backwards compatibility with WordPress in certain cases like when there is an empty paragraph block.

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 using the code editor.
• 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.
New Feature: core-data now supports a hook for syncing entities with controlled block/inner block content.
New Feature: InnerBlocks now supports a controlled mode for syncing blocks with other APIs.

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. .

[epiqueras]

Following discussions, I removed useEntitySyncedInnerBlocks as it was felt that the relationship to InnerBlocks was too indirect so we compromised on the controlled InnerBlocks approach from previous PRs, but now with useEntityBlockEditor for control encapsulation.

[epiqueras]

I added support for Template Part blocks that are user-inserted and not provided from a file with initial slug and theme attributes.

The placeholder (with preview) pattern allows you to choose an existing template part from a file or CPT post, by slug and theme, or create a new one:

https://youtu.be/XOI5MldSC0k

[gziolo]

I have a few questions from the perspective of the consumer of the new API. I extracted bits which are relevant for those writing blocks:

export default function TemplatePartInnerBlocks() {
 	const [ blocks, onInput, onChange ] = useEntityBlockEditor(
 		'postType',
 		'wp_template_part',
 		{
 			initialEdits: { status: 'publish' },
 		}
 	);
 	return <InnerBlocks blocks={ blocks } onInput={ onInput } onChange={ onChange } />;
 }

return (
 	<EntityProvider kind="postType" type="wp_template_part" id={ postId }>
 		<TemplatePartInnerBlocks />
 	</EntityProvider>
 );

There is some duplication between the setup for the EntityProvider and useEntityBlockEditor hook. They both define the same kind and type. The provider picks the proper post type with id, but the hook defines some overrides to apply when the hook gets applied. It makes me wonder whether this could be consolidated inside EntityProvider call. To be fair, I don't know anything about underlying APIs but as a someone using API I would prefer something like:

``js
export default function TemplatePartInnerBlocks() {
const [ blocks, onInput, onChange ] = useEntityBlockEditor();

return <InnerBlocks blocks={ blocks } onInput={ onInput } onChange={ onChange } />;

}

```js
const initialEdits = { status: 'publish' };
return (
 	<EntityProvider kind="postType" type="wp_template_part" id={ postId } initalEdits={ initialEdits }>
 		<TemplatePartInnerBlocks />
 	</EntityProvider>
 );

Well, it's had to tell if it is doable, but @epiqueras, you will know better :)

Can you even call useEntityBlockEditor for the same entity provider multiple times with different initialEdits? It seems like something which might lead to unexpected results taking into account that it would depend on the rendering order.

I have also much less concerning observation about the return result of useEntityBlockEditor hook. At the moment it follows the pattern established by React hooks like useState or useReducer where they return a tuple – the value and setter. I assume that in this case, we can expect that all 3 items from the array will be applied to the InnerBlock components as props, so it might be easier to use an object to simplify the application:

export default function TemplatePartInnerBlocks() {
 	const entityProps = useEntityBlockEditor();

 	return <InnerBlocks { ...entityProps } />;
 }

Still, in the case where you would want to use only 1 or 2 items, it's still easier to read destructuring of the object:

const [ ,, onChange ] = useEntityBlockEditor();

vs

const { onChange } = useEntityBlockEditor();

[epiqueras]

There is some duplication between the setup for the EntityProvider and useEntityBlockEditor hook. They both define the same kind and type.

This was an intentional design decision. Each entity kind-type pair should have its own context, otherwise we can't have a block or multiple sibling blocks that read from different entity types. E.g. a Site Title block next to a Post Content block.

Can you even call useEntityBlockEditor for the same entity provider multiple times with different initialEdits? It seems like something which might lead to unexpected results taking into account that it would depend on the rendering order.

Yes, it shouldn't be a problem as it's usually used for idempotent edits like here for the template part auto-publishing behavior. If it becomes problematic, we can always make the initial edits only apply if the targeted entity record has not already been edited.

I have also much less concerning observation about the return result of useEntityBlockEditor hook. At the moment it follows the pattern established by React hooks like useState or useReducer where they return a tuple – the value and setter. I assume that in this case, we can expect that all 3 items from the array will be applied to the InnerBlock components as props, so it might be easier to use an object to simplify the application:

I tried that approach as well. I don't have strong feelings about it, but I convinced myself that the tuple approach was better with the following line of reasoning:

Getter-setter tuples are the hooks convention and they force you to be explicit about what a hook is returning and what its for. Returning an object and spreading is a compromise that should only be made when the number of values is so big that it's impractical to use a tuple or when the number or order of values is not known at authoring time.

What do you think?

[mtias]

It's a bit odd to have this exposed here as an editable attribute since there is already "current theme" context that can be implicit.

[epiqueras]

Yes, but you still need to be able to add template parts from non-active themes that you installed and customized.

I was thinking of filling it with the current theme by default, and maybe hiding it behind some sort of "advanced" checkbox/option?

What do you think?

[mtias]

Yes, pre-filling and hiding by default sounds good. The mixing is a bit more complex and should not be immediately exposed.

[jorgefilipecosta]

I tested this PR with success and it seems to be in a very good direction, awesome work @epiqueras 👍
I noticed some issues but nothing major:

• Inserting a template part even if save does not happen creates a new post. Should we create the post on save?
• Update is always available as soon as a template part is requested even if changes do not happen.
• If I type a single character in a paragraph inside a template part, the template part does not become “dirty” and the save does not happen if I type multiple characters fast the template part becomes "dirty".

[jorgefilipecosta]

The attributes of the template part block can contain postId, slug, and theme e.g:
<!-- wp:template-part {"postId":28477,"slug":"st1","theme":"twentytwenty"} /-->
There is some redundancy here, the postId would be enough to retrieve the other two attributes.

[epiqueras]

Template parts that load from files provided by themes can't have post IDs before being customized.

See #18736.

[jorgefilipecosta]

Yes that's true I think slug would still exist. What I was saying is when we reference by postId, I think we should not save the slug and theme attributes otherwise we are saving redundant information. For example, the user or a plugin may change the slug of that post or the theme field and in that case, the information stored in attributes referencing that postId would become obsolete.
To reference template parts loaded from files we would still use the slug.

[epiqueras]

For example, the user or a plugin may change the slug of that post or the theme field and in that case, the information stored in attributes referencing that postId would become obsolete.

That, or deleting the post, would be a way to have instances of the template part go back to rendering the contents from an original theme file.

[jorgefilipecosta]

Are we going to support the use case of referring the same template part multiple times in a post?
When I tried to do it the block controls and selection appear multiple times.

[mtias]

Good question. Template parts can be inserted multiple times: https://developer.wordpress.org/reference/functions/get_template_part/

[epiqueras]

Yes, we need to support this.

This is happening because the inner blocks have the same client IDs. I am pretty sure this wasn't a problem before the recent selection refactoring and inclusion in undo history, so we should be able to fix it pretty easily.

@ellatrix will have a better idea of what to do since she worked on that, but I don't think this is a blocker for this PR.

[jorgefilipecosta]

When I select the template part using the block navigator the height of the block does not include its content:

[epiqueras]

I can't replicate this. Is there anything specific you are doing that causes it?

I also don't think it's related to this PR since we are just returning the regular InnerBlocks markup.

[epiqueras]

Thanks for the review! 😊

• Inserting a template part even if save does not happen creates a new post. Should we create the post on save?

This is doable, but it would make the code a lot more complex, do you think it's a worthy optimization, or are you suggesting it might somehow detract from the user experience. In any case, I think we can deal with it in follow up work.

• Update is always available as soon as a template part is requested even if changes do not happen.

Yes, the initial value of the inner blocks dirties the post. We'll need something like a non-dirtying version of replaceInnerBlocks, but I didn't want to block this PR with a very debatable change like that.

• If I type a single character in a paragraph inside a template part, the template part does not become “dirty” and the save does not happen if I type multiple characters fast the template part becomes "dirty".

Good catch! I got onChange and onInput mixed up. It's fixed now.

[jorgefilipecosta]

We are using block_editor_settings filter as a way to perform create_auto_draft_for_template_part_block before the editor is loaded. I wonder if we have a specific action that allows executing some code when the block editor is being loaded.

[epiqueras]

We'll need to extend the settings with template IDs for the editor very soon, so I think it makes sense to keep all the logic here.

[jorgefilipecosta]

Would it make sense to mark blocks property as experimental until FSE is stable?
Blocks property acts exactly like a template -- We prefill an InnerBlocks area, future changes to the property are not taken into consideration. The only difference compared to templates is that blocks is a normal block structure with attributes cliendIds etc while the template uses a specific template structure.
I wonder if the need to have this type of prefilling will be common that worths being part of InnerBlocks or if it is something specific that should be part of the template part bock itself.

[epiqueras]

A lot of blocks will use it: Post Content, Reusable, Query, basically any blocks that save their children or a representation of them somewhere other than the current post.

I'm comfortable with it not being experimental, but we can change it if you have reservations about the API.

[jorgefilipecosta]

A lot of blocks will use it: Post Content, Reusable, Query, basically any blocks that save their children or a representation of them somewhere other than the current post.

I'm comfortable with it not being experimental, but we can change it if you have reservations about the API.

Hi @epiqueras,

The blocks that use this API don't seem "normal" blocks and have a very specific need I'm not sure if this API is useful for normal InnerBlocks use cases. I guess in the future we may have an alternative abstraction to InnerBlocks used by post content, reusable, etc... specific for these blocks (it may use InnerBlocks and handle the replacing, etc..). For now, I think it would be safer to have an experimental API until more blocks are implemented and we have more knowledge on the direction.

[epiqueras]

I'll make it experimental for now.

[jorgefilipecosta]

Normally, in this case, I would add _setSlug and setHelp as dependencies althougth I'm not expecting these functions to change. I wonder if it is totally safe to not include them in the dependencies.

[epiqueras]

React cannot change these setters, and it's become an implicit convention not to list them in dependencies. I've also seen them always be listed. I think not listing them makes things easier to read.

Build tools will handle this at some point, so I don't have a strong opinion.

[jorgefilipecosta]

From now on, I will follow the same approach and not list them 👍

[youknowriad]

I think we've been avoiding function/variable names with _ :)

[epiqueras]

Really? Why? We've merged quite a bit of code with them. It's a standard convention for unused/intermediate variables. Forcing a different variable name usually ends up awkwardly.

[epiqueras]

Name the callback in a way which encompasses all of the expected effects, e.g. setSlugAndHelp

In this particular case, this makes the most sense.

Name the callback in a way which represents its intended use, e.g. onSlugChange

• I don't often encourage this since it falls into a trap of its own in that it is not descriptive of what we expect to happen when the slug is changed.

Agreed

Don't name the callback at all, e.g. anonymous function assignment in the TextControl attribute

It would make the code harder to follow here.

Or, if possible, remove the help state altogether, if it could be computed on-the-fly as a derivation of the slug (similar to this old guideline)

We would still need some state because help can also be an error message. We could just set the status instead and compute help on the fly, but that might be harder to read.

What are your thoughts on things like this?

const option = options.find( _option => _option === 'someOption' )

[aduth]

What are your thoughts on things like this?

const option = options.find( _option => _option === 'someOption' )

I think it works a little better, but it could still be prone to some confusion in that: We call the value an "option", but in each occurrence, it represents a different thing:

• Either the result of an Array#find iteration, presumably having some meaning based on the context in which it's used (unclear in this example).
• A candidate of the Array#find iteration, one of the options which could ultimately become the resulting value.

We could choose to make this distinction clear in the naming, even if it ends up being more verbose. Because otherwise, we leave it to the reader to try to decipher this difference, which we should seek to avoid if there's a simple alternative.

const selectedOption = options.find( option => option.isSelected );

const currentRoute = routes.find( routeCandidate => routeCandidate.isMatch( currentURL ) );

(The "Candidate" suffix in this latter example isn't strictly necessary and arguably redundant, though maybe in some better examples a similar prefix/suffix help to clarify how it is a value under consideration)

In some cases, I think the thought experiment might lead us to call it something entirely different altogether, more representative of how it ends up being used.

const page = posts.find( post => post.type === 'page );

I don't recall specifically where the conversation took place, but there's a similar issue with how we deal with variable shadowing in useSelect, where something like what you're suggesting might be appropriate enough if even to consider that the selectors are often the means-to-an-end of some resulting value.

const isActive = useSelect( ( select ) => {
    const { isActive: _isActive } = select( 'foo' );
    return _isActive();
} );

I know @ellatrix has alternatively worked around this by defining the selector separately:

function selector( select ) {
    const { isActive } = select( 'foo' );
    return isActive();
}

const isActive = useSelect( selector );

(I'm not sure if this is exactly right, though maybe it is.. depending on const and if variable hoisting is considered as being "shadowed")

[epiqueras]

I think I would also go with:

const selectedOption = options.find( option => option.isSelected );

or

const foundOption = options.find( option => option.isSelected );

I don't recall specifically where the conversation took place, but there's a similar issue with how we deal with variable shadowing in useSelect, where something like what you're suggesting might be appropriate enough if even to consider that the selectors are often the means-to-an-end of some resulting value.

Yes, we could also prefix it with "select" or "get".

(I'm not sure if this is exactly right, though maybe it is.. depending on const and if variable hoisting is considered as being "shadowed")

It's not shadowing, but it makes code verbose if you need closures.

I think we should have lint rules for these things. It's definitely inconsistent across the codebase.

[aduth]

I think we should have lint rules for these things. It's definitely inconsistent across the codebase.

For which, specifically?

As a baseline, we already have no-shadow enabled.

Regarding the underscore prefixing, I'm not sure we want to go so far as to forbid it, though I think in the course of this discussion it's become clearer that there are often better choices which can be made.

One potential interoperability concern would be with TypeScript checking, since it actually has special treatment of the underscore prefix when considering unused arguments (microsoft/TypeScript#9458, microsoft/TypeScript#9464, microsoft/TypeScript#29202). (Aside: I don't love how this works, but it is what it is)

Or were you thinking about something specific to the selector callback for useSelect ?

[epiqueras]

For which, specifically?

• Prohibit bypassing no-shadow with an underscore prefix.
• Enforce "select" prefix when shadowing a variable with a selector.

[epiqueras]

I've addressed the latest feedback. Let me know if I missed anything 😄

[jorgefilipecosta]

Thank you for the iterations and answer al the questions @epiqueras I think this PR is ready 👍 Awesome work here!
I noticed a regression that would be good to fix before the merge (I did not see this problem on the previous tests). I did the following steps to reproduce it:
Created a template e.g: single.
Added a paragraph and a template part in that template.
Added a paragraph inside the template part.
Saved everything.
Typed multiple characters in the paragraph inside the template part.
Saved the template and the template part.
Reloaded the post and verified that not all the characters I typed are saved (some were lost).

[epiqueras]

Thanks for the reviews 😄

I fixed that here: 5804aae.

[epiqueras]

Update is always available as soon as a template part is requested even if changes do not happen.

Follow up: #19521.

[youknowriad]

so this is basically the "value" prop right?

[epiqueras]

Yes, but it follows the BlockEditorProvider in the naming style.