Gutenberg: Controlled Inner Blocks Clear On Mount

[epiqueras]

Something has regressed with controlled inner blocks where initial values are overwritten with an empty array shortly after mount.

This is noticeable when using the Template Part block placeholder to insert existing template parts. The preview works, but the block is empty after insertion.

[epiqueras]

I just bisected and found the issue was introduced by #21467.

It's a race condition so it was a bit hard to track down.

When InnerBlocks mounts in controlled mode, it sets its initial value in the block-editor store using replaceInnerBlocks. Then, the listener in the block editor provider syncs it with the editor store. Keep in mind that this listener is asynchronous.

21467 introduced a resolver for getEditedEntityRecord. Template parts trigger it when getting their initial content. Resolvers dispatch START_RESOLUTION synchronously. That triggers an update in the editor store, which in turn rerenders the block editor before the listener mentioned above has had a chance to run. InnerBlocks then gets an empty array and replaces the actual initial blocks that were already in the block-editor store.

I see a few solutions:

• Remove the resolver for getEditedEntityRecord. This is the simple one, but it sort of treats the symptom without curing the disease.
• Make resolver resolution asynchronous. I.e., Fire START_RESOLUTION after a timeout. This seems like the cleanest to me, but it will hurt performance.
• Provide an escape hatch to InnerBlocks to trigger the block editor provider's listener synchronously. This is hacky but won't affect performance much.

I need to look deeper into this as I just briefly sleuthed this afternoon.

@aduth @youknowriad @Addison-Stavlo

What do you think? Does this make sense? Which of the options do you think is best?

[epiqueras]

@aduth @youknowriad or maybe we should force the registry to queue listeners in the order that updates happen? We would need to implement store specific subscriptions for that, though.

[aduth]

• Make resolver resolution asynchronous. I.e., Fire START_RESOLUTION after a timeout. This seems like the cleanest to me, but it will hurt performance.

Would it actually need to be a measurable duration of time? Or simply be scheduled to occur after the current call stack (i.e. setTimeout( fn, 0 ))? If the latter is sufficient, I can't imagine it would actually harm performance much, at least not in a way that could be perceived by a user.

I'd want to take a closer look at the specific implementation, since the flow is hard for me to grasp. I do feel there could be some benefit to making resolvers to be scheduled to start asynchronously, in reducing the chance for these sorts of conflicts, making the behavior more predictable, and reducing reliance on synchronous behaviors.

On the other hand, the syncing flow for InnerBlocks has always seemed rather clunky to me. Again, not sure how relevant or possible it would be to refactor, but if it's an option to solve the issue, I think it could be worth exploring.

[epiqueras]

Would it actually need to be a measurable duration of time? Or simply be scheduled to occur after the current call stack (i.e. setTimeout( fn, 0 ))? If the latter is sufficient, I can't imagine it would actually harm performance much, at least not in a way that could be perceived by a user.

The latter is sufficient. I was more worried about the number of tasks scheduled on load when a lot of resolvers fire. I'll put it up on a PR.

I'd want to take a closer look at the specific implementation, since the flow is hard for me to grasp. I do feel there could be some benefit to making resolvers to be scheduled to start asynchronously, in reducing the chance for these sorts of conflicts, making the behavior more predictable, and reducing reliance on synchronous behaviors.

Yeah, I imagine there could be other cases where this could be a problem, and intuitively, I expected it to work this way since resolvers are like a forked task.

On the other hand, the syncing flow for InnerBlocks has always seemed rather clunky to me. Again, not sure how relevant or possible it would be to refactor, but if it's an option to solve the issue, I think it could be worth exploring.

I agree that could be improved, but perhaps this issue is something we want to fix regardless?

[Addison-Stavlo]

The latter is sufficient.

Awesome! I think the timeout would be a simple and effective solution in that case. Nice job tracking this one down.

[epiqueras]

I looked more into this, and the timeout doesn't work because the cause is actually a bit deeper in the code paths. I added comments to the relevant snippet to explain:

export function* resetEditorBlocks( blocks, options = {} ) {
    const {
        __unstableShouldCreateUndoLevel,
        selectionStart,
        selectionEnd,
    } = options;
    const edits = { blocks, selectionStart, selectionEnd };
    // First Run: `[ core/template-part: [] ], undefined`.
    // Second Run: `[ core/template-part: [ ...initialBlocks ] ], false`.
    console.log( blocks, __unstableShouldCreateUndoLevel );
    if ( __unstableShouldCreateUndoLevel !== false ) {
        const { id, type } = yield select( STORE_KEY, 'getCurrentPost' );
        const noChange =
            ( yield select(
                'core',
                'getEditedEntityRecord',
                'postType',
                type,
                id
            ) ).blocks === edits.blocks;
        // First Run: `false`.
        // Second Run: Doesn't reach this code.
        // getEditedEntityRecord having a resolver makes
        // the execution from the first run reach this point
        // after the second run finishes. This overwrites the
        // blocks with a stale value. A stale value without
        // template part inner blocks which causes the issue
        // described here.
        console.log( noChange );
        if ( noChange ) {
            return yield dispatch(
                'core',
                '__unstableCreateUndoLevel',
                'postType',
                type,
                id
            );
        }
        // We create a new function here on every persistent edit
        // to make sure the edit makes the post dirty and creates
        // a new undo level.
        edits.content = ( { blocks: blocksForSerialization = [] } ) =>
            serializeBlocks( blocksForSerialization );
    }
    yield* editPost( edits );
}

A simple solution that I'll implement for now is to make a synchronous select data control like we used to have before.

But, I think that this problem could surface elsewhere, so the solution should be more general. We should think about canceling stale action execution. That also raises the questions of how to define and continue with any cleanup operations, though.