Fix issues with inserting the same inner block controller more than once #24180
Conversation
noahtallen
added
[Type] Bug
|
Size Change: +743 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
|
My current guess for the selection issue is that it has something to do with the onFocus event firing in RichText, but there are two RichText components for the same thing, and they both fight for control of the selection which ends up getting set to 0. At any rate, there seems to be about 10 dispatches of set selection state when selecting into the component. Or something like that. At any rate, I've confirmed that it is not related to |
|
Nice work! |
ZebulanStanphill
added
the
Needs Technical Feedback
|
I ran into this earlier. Glad you're working on a fix. |
|
I've been thinking about alternate approaches here which would map incoming blocks from an entity to fresh block IDs for use within the block editor. I'll try to post a more detailed dive into my thoughts so far tomorrow. I think mapping the blocks to fresh IDs when there are > 1 instance of the same inner block controller will fix the issues we have, but it will be difficult to handle incoming/outgoing states without race conditions. |
|
To help us understand the situation more (and to help me think about the problem), I created a flowchart sort of thing which you can view here: https://whimsical.com/7jzmmDXxDDHVeidLthhmhZ. Screenshots and descriptions below. I hope this is helpful to anyone who might not understand the issue yet and wants to help out :) Description of block sync mechanism with template parts:This flowchart describes how the template part entity is parsed from the CPT, added to an entity provider, and synced to any template part block which references the given template part entity. To the left is a description of when changes are synced to/from the different components at each level:
An example with blocksThis chart describes a situation where you have a template part entity with the parsed block client IDs The problem with having duplicated blocks is evident when looking at the const parents = {
a: 'tp1',
b: 'tp2',
c: 'tp3',
};As you can see, I can only say that the block with clientId There are several other bits of the block editor store which behave in similar ways, where having unique block client IDs is assumed. In some places, this is actually helpful, because on screen, the block with clientID a needs to behave the same way after it is already inside of the template part block (e.g. it has the same children everywhere, the same attributes, same selection, etc). The main problem is just hierarchy, positioning, and race conditions. An example chain of actions: editing a template part's inner blocksThis diagram traces what happens when you make a modification to, say, the block with clientId Note: start at the "START HERE" button at the bottom left.
As you can see from the diagram, the ultimate effect of the Solutions
So far, it seems like number 2 is the best option, and I've been exploring ways to accomplish this. I should be able to post more about that soon. |
|
Thinking about solution two for a bit, I think we can make it work. This solution would basically map the blocks from the entity to new block clientIDs at the
Thinking about implementation details, we would have a global data structure to track which entities are already being tracked: // tracks which entities are in the block editor
const entityTracker = {
entityIdentifier: string[] // an array of strings identifying useBlockSync instances
};
// In useBlockSync
useEffect( () => {
// on component mount, make sure we register ourselves as something tracking the entity.
entityTracker[ entityIdentifier ].push( selfId );
return () => entityTracker[ entityIdentifier ].remove( selfId );
}, [ entityIdentifier ] );
// When the incoming block value from the entity changes:
// If there is more than one thing listening to this entity, we need to map the blocks
if ( entityTracker[ entityIdentifier ].length > 0 ) {
blocks = mapBlocksToLocalIds( blocks );
// then send the blocks to the local block editor.
}
// When the block editor updates with new blocks:
if ( entityTracker[ entityIdentifier ].length > 0 ) {
blocks = mapBlocksToEntityIds( blocks );
// then send the mapped blocks to onChange.
}Then we would need a local data structure to match which blocks have been mapped: const entitiesToLocal = {};
const localToEntities = {};
function mapBlocksToLocalIds( blocks ) {
// Note: actual implementation would have to account for nested innerBlocks.
return blocks.map( ( block ) => {
if ( ! entitiesToLocal[ block.id ] ) {
const localId = new UID(); // Just has to be a unique string of some sort.
entitiesToLocal[ block.id ] = localId;
localToEntities[ localId ] = block.id;
}
return {
...block,
id: entitiesToLocal[ block.id ],
};
} );
}
function mapBlocksToEntityIds( blocks ) {
// Note: actual implementation would have to account for nested innerBlocks.
return blocks.map( ( block ) => {
if ( ! localToEntities[ block.id ] ) {
// This entityId could be more tricky when inserting new blocks.
// Would need to verify how clientIds for new blocks are generated,
// and if that would be compatible with change here.
const entityId = new UID();
localToEntities[ block.id ] = entityId;
entitiesToLocal[ entityId ] = block.id;
}
return {
...block,
id: localToEntities[ block.id ],
};
} );
}I think the possible rough edges here are the following:
|
|
@noahtallen some variations of these charts and explanations should go into the "architecture" docs of the repository to explain how the full site editor works :) |
|
Currently, it fixes the problem where duplicated instances are deleted immediately when you insert them. There are still several issues though. I haven't spent any time looking into them yet, so far was just able to implement the rough code we have now.
|
noahtallen
force-pushed
the
try/fix-inner-blocks-insertion
branch
from
172a606 to
306add6
Compare
noahtallen
requested review from
ajitbohra,
Soean and
talldan
as code owners
| @@ -67,7 +67,8 @@ | |||
| "refx": "^3.0.0", | |||
| "rememo": "^3.0.0", | |||
| "tinycolor2": "^1.4.1", | |||
| "traverse": "^0.6.6" | |||
| "traverse": "^0.6.6", | |||
| "uuid": "^7.0.2" | |||
There was a problem hiding this comment.
Note: this is used elsewhere in gutenberg
| @@ -26,7 +26,7 @@ import getBlockContext from './get-block-context'; | |||
| import BlockList from '../block-list'; | |||
| import { BlockContextProvider } from '../block-context'; | |||
| import { useBlockEditContext } from '../block-edit/context'; | |||
| import useBlockSync from '../provider/use-block-sync'; | |||
| import useBlockSync from '../use-block-sync'; | |||
There was a problem hiding this comment.
Moved use block sync to its own directory
| } ) { | ||
| const registry = useRegistry(); | ||
| const instanceId = useRef( uuid() ).current; |
There was a problem hiding this comment.
The withInstanceId/useInstanceId won't work here because they rely on mapping an object (e.g. component) to a new ID. This approach doesn't work with custom hooks directly. (it might be useful to move this logic to that package so that it does, though)
| @@ -84,6 +102,14 @@ export default function useBlockSync( { | |||
|
|
|||
| const pendingChanges = useRef( { incoming: null, outgoing: [] } ); | |||
|
|
|||
| useEffect( () => { | |||
There was a problem hiding this comment.
could possibly move this to its own custom hook
| @@ -199,6 +246,7 @@ export default function useBlockSync( { | |||
| ) { | |||
| pendingChanges.current.outgoing = []; | |||
| } | |||
| // TODO: problematic if getBlocks has alternate client IDs | |||
| } else if ( getBlocks( clientId ) !== controlledBlocks ) { | |||
There was a problem hiding this comment.
Need to fix this check. Theoretically, if blocks are being mapped to local IDs, then getBlocks will return one array with local IDs, but the controlledBlocks will use the IDs which come from the entity. As a result, the references will always be different.
There was a problem hiding this comment.
I don't think this is causing any bugs, though. It's probably just here for performance
Redux state and selectors in general rely a lot on memoization, what I'm saying is that when |
Hm. I'm following a little better. But to clarify, do you mean that we would stop generating new objects during the mapping process? Every single block needs to receive a new clientID regardless of whether it is different. Every client ID needs to be unique locally. In this scenario, do we simply mutate the objects instead of creating new ones? But would that work? How about this scenario:
|
No, that's not what i meant, I mean we continue generating new objects but we keep two lists of objects (one external and one internal) and when doing the sync, if the previous version of a block is strictly equal to the new version, it means that particular block didn't change, so instead of creating a new instance for the mapped version, pick the previously used one from the other list. This optimization is useful to avoid extra re-rendering because our usage of React and redux rely on memoization. That said, I'm not sure how complex it is to implement. |
|
Oh that's really interesting! It could help performance, not by reducing the cost of the array iteration, but by improving memoization ability across the editor UI. |
Also removes need for robust instance management
noahtallen
force-pushed
the
try/fix-inner-blocks-insertion
branch
from
02d43ac to
78d170c
Compare
|
c976304 attempts to persist the object references if the block has not changed. I'm pretty sure that the logic is correct, but I'm noticing a big performance improvements. The editor becomes noticeably sluggish after a point. 🤔 |
|
One thought is that we might not be realizing all of the performance gains we could be with this approach. Let's assume that everything is working correctly as outlined in @youknowriad's comment above. Even if this is correct, the local i.e.: Then, say an update happens to All as expected, right? But now we do this: And the replace inner blocks action is going to first remove all existing blocks, and then add all of |
| // TODO: useState vs useRef here? Unsure if a change in the manager should | ||
| // re-run useBlockSync, or if pointing at the ref works fine. | ||
| const [ manager, updateManager ] = useState( | ||
| createBlockClientIdManager( assocateWithEntity( entityId ) ) | ||
| ); | ||
|
|
||
| const entityIdRef = useRef( entityId ); | ||
|
|
||
| // This side effect creates a new block client ID manager if and only if the | ||
| // component starts using blocks from a new entity. Since consumers require | ||
| // the manager before this effect would be run, we initalize the manager | ||
| // above and then avoid initalizing a new one the first time the effect runs. | ||
| useEffect( () => { | ||
| if ( entityIdRef.current !== entityId ) { | ||
| updateManager( | ||
| createBlockClientIdManager( assocateWithEntity( entityId ) ) | ||
| ); | ||
| entityIdRef.current = entityId; | ||
| } | ||
| }, [ entityId ] ); |
There was a problem hiding this comment.
I might not know what I'm talking about, but couldn't this whole thing just be replaced with:
const manager = useMemo(
() => createBlockClientIdManager( assocateWithEntity( entityId ) ),
[ entityId ]
);
The reasoning sounds correct, so if it's true maybe the optimization we did is not the best one or the right one. I guess ideally we should find a way to avoid generating a new cache key in these cases right? |
|
Yeah, totally. it sounds like this sync behavior should be happening at some level inside the block-editor store. Like, when I do |
|
Hey, @noahtallen I just remembered this PR :) It seems we need to find a path forward here (even an imperfect one), what's your current thoughts? |
|
Hey! My goal for this week is to push this PR over the finish line :D My current thought is to do the syncing inside the block-editor redux code. Basically, something like the following:
The idea here is to move the "sync edits across all duplicated blocks" part of the problem into the reducer, rather than relying on an outbound sync to the entity store, and then back into the block-editor store again. This would really cut down on the amount of state updates, which would probably work very well for performance. |
|
I spent some time thinking about this recently. It's a tricky. The Problem:To summarize the solution which is currently implemented in this PR: Instead of inserting duplicated client IDs into the block tree in different places, we instead map all synced blocks to new IDs whenever we put the blocks into the block editor. Then, when we make an edit, we sync the blocks back to its source ID before telling the entity about the update. When that happens, the entity can inform all the other listeners of the new blocks, which then sync to new IDs, and are then inserted. So the sync process in short is:
As you can tell, this approach has us recursing through the entire slice of the block tree under a certain template part multiple times. First on the outgoing edit, and then again up to N more times. Not to mention some amount of latency between getting the update into the entity store, and then back into the block editor store. On top of that, we have to delete the entire subtree N times, and then re-insert it N times. This is unfortunate, because the sync happens on every edit.... So, on every keystroke, you are iterating through every single nested block of the local template part and doing a fair amount of work. Basically, it's a performance problem. Though this approach works to solve the bugs we currently encounter, it's pretty heavy-handed! One thing I was thinking: do we have to use A Path ForwardWhat's the real problem we're solving here? It's essentially how do we show one block multiple times in the block tree? My concept to solve this problem would involve a way to
|
|
@noahtallen This makes sense to me, so you're saying that basically we add a "block aliases" state to the block-editor store right? I was also thinking about this PR while working on #26866 and it seems having the state being made of atoms, might make this easier as well. (not that it should wait for the other PR to land) |
Yep, basically! And then account for that in the various block operations. I still need to figure out where exactly the data would go, but the model is something like: I imagine these props don't necessarily need their own object, but could be added as block properties elsewhere.
I was also thinking about that! I'm not exactly sure how it fits in (I haven't looked at it closely), but it does seem like better atomicity in the stores would be helpful. For example, the edit: though, we would still need the alias state, because the current approach in master just inserts duplicate clientIds everywhere, which doesn't work with parts of the state. For example the |
|
One challenge with this new approach is "what happens to the aliases when the sources are removed"? For example, say you insert a template part block. This becomes the source. Then you insert another template part block referencing the same entity. These child blocks become "aliases" of the source child blocks. Now, what if I remove the first template part block? I'm not actually deleting the blocks from the template part entity, I'm just removing the template part block from the template. In this case, we need to handle a couple of extra things:
|
Thought about this a bit more. I think we can basically do this:
This way, all of the aliases continue working exactly as before, and we still have something to sync edits in and out. |
|
I've started to write the approach this PR: #27084 |
|
superseded by #27885 Thanks for you work on these issues @noahtallen |
Description
Resolves #22639. You can now add more than one template parts of the same type to the editor. (or anything which shares the same data source as an inner block controller.)
I wrote a description for this approach in this comment. TLDR is that in cases where two different inner block controllers share the same
entityId(and therefore data source), we map the blocks in all duplicated instances to different local clientIds. This way, there are no duplicated clientIds stored in the block editor.Current issues:
How has this been tested?
Locally, in edit site.
Screenshots
TBD.
Types of changes
Bug fix
Checklist: