Size Change: +47 B (0%)

Total Size: 845 kB

_{compressed-size-action}

Verify that #21804 is no longer an issue; that you can insert existing template parts on the full site editing experiment as expected.

Since it took me a while to figure out the testing instructions:

1. (Prerequisite) Activate and save "Enable Full Site Editing" from Gutenberg > Experiments
2. Navigate to Posts > Add New
3. Insert a "Template Part" block
4. Enter "header" for Slug and "twentytwenty" for Theme
5. Press "Choose"
6. Note the text "Header Template Part" appears
   • In master, it's wrongly empty prompt "Start writing or type / to choose block"

But, I think that this problem could surface elsewhere, so the long-term 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.

Do we want a new public API member syncSelect then? Should it be __unstable? Or local to the editor package like we do with some similar controls?† Or is there valid use for this beyond addressing the immediate issue here?

† I believe this example isn't done this way on purpose, merely that it was implemented before the package was generalized.

Do we risk any inaccurate value from getEditedEntityRecord by not awaiting the resolver? I guess if it wasn't awaiting anything before, this would restore that previous behavior? (Which doesn't speak to whether or not it should be waiting for e.g. getEntityRecord).

Not sure if this was introduced here or in #21804, or if it's always been an issue, but I see a lot of duplicate toolbar and inspector controls for a freshly-inserted Template Part block. Plus, when trying to interact with the block, it locks up my editor.

It doesn't seem to be an issue when saving and reloading.

Edit: Testing it further, it might only be an issue when inserting a Template Part block when there are already Template Part blocks in the post for the same template part. In my case, I effectively had three of the same template part.

But, I think that this problem could surface elsewhere, so the long-term 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.

Do we want a new public API member syncSelect then? Should it be __unstable? Or local to the editor package like we do with some similar controls?† Or is there valid use for this beyond addressing the immediate issue here?

On the topic, while I don't think it's inaccurate to name it as such, the specific use of "sync" leads me to think of it differently, in-fact more like how it actually behaves today. When I see the name, I consider it like Node's readFileSync, where it would wait before proceeding. The actual behavior is more like don't wait, just use state as it exists at the moment.

Do we risk any inaccurate value from getEditedEntityRecord by not awaiting the resolver? I guess if it wasn't awaiting anything before, this would restore that previous behavior? (Which doesn't speak to whether or not it should be waiting for e.g. getEntityRecord).

No, it should be fine.

Not sure if this was introduced here or in #21804, or if it's always been an issue, but I see a lot of duplicate toolbar and inspector controls for a freshly-inserted Template Part block. Plus, when trying to interact with the block, it locks up my editor.

Yes, this is because they are the same template parts, so they share block instances/IDs. We need to fix this.

On the topic, while I don't think it's inaccurate to name it as such, the specific use of "sync" leads me to think of it differently, in-fact more like how it actually behaves today. When I see the name, I consider it like Node's readFileSync, where it would wait before proceeding. The actual behavior is more like don't wait, just use state as it exists at the moment.

What would you call it? noResolverSelect seemed too wordy.

I'd probably prefer to hide syncSelect from the public API in its current form, but otherwise it appears to fix the issue in my testing.

Agreed, I'll make it unstable for now. But, we need to arrive at a long-term solution because this will also be a problem for third parties.

Separately, we should consider some automated testing coverage for this behavior, especially since it's fixing a regression.

Yes, I want to start with e2e tests for template parts, which should indirectly test a lot of the new behaviors.

What would you call it? noResolverSelect seemed too wordy.

I guess it doesn't really matter much if it's going to be internal / unstable. I don't really have great name alternatives in mind, maybe something reflecting the "immediacy" or "current state" aspects of the behavior. I also kinda wished it would be an option of the existing select, but given the variadic nature of the existing function, we can't easily add an "options" argument. I'm not too worried about changing anything.

I'd probably prefer to hide syncSelect from the public API in its current form, but otherwise it appears to fix the issue in my testing.

Agreed, I'll make it unstable for now. But, we need to arrive at a long-term solution because this will also be a problem for third parties.

Yeah, but unless I'm mistaken, we don't want anything like this to be the end solution, which also makes it a perfect use-case for __unstable. The fact that this became an issue isn't a result of us explicitly wanting to not wait for resolvers attached to the selectors called, it's just that we unintentionally depended on that behavior. I think what you've suggested about cancelling or queueing these actions / resolvers to occur in a predictable way would be much more durable, and I'd doubt it would require the syncSelect introduced here.

I guess it doesn't really matter much if it's going to be internal / unstable. I don't really have great name alternatives in mind, maybe something reflecting the "immediacy" or "current state" aspects of the behavior. I also kinda wished it would be an option of the existing select, but given the variadic nature of the existing function, we can't easily add an "options" argument. I'm not too worried about changing anything.

Yeah, I reached for that first as well.

Yeah, but unless I'm mistaken, we don't want anything like this to be the end solution, which also makes it a perfect use-case for __unstable. The fact that this became an issue isn't a result of us explicitly wanting to not wait for resolvers attached to the selectors called, it's just that we unintentionally depended on that behavior. I think what you've suggested about cancelling or queueing these actions / resolvers to occur in a predictable way would be much more durable, and I'd doubt it would require the syncSelect introduced here.

Exactly, that's what I was referring too.