-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Editor: Add local change detection. #19741
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,13 @@ import { | |
identity, | ||
difference, | ||
omitBy, | ||
pickBy, | ||
} from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import isShallowEqual from '@wordpress/is-shallow-equal'; | ||
import { combineReducers } from '@wordpress/data'; | ||
import { isReusableBlock } from '@wordpress/blocks'; | ||
/** | ||
|
@@ -373,6 +375,8 @@ function withPersistentBlockChange( reducer ) { | |
|
||
return ( state, action ) => { | ||
let nextState = reducer( state, action ); | ||
nextState.persistentChangeRootClientId = | ||
action.type === 'UPDATE_BLOCK_ATTRIBUTES' && action.rootClientId; | ||
|
||
const isExplicitPersistentChange = | ||
action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT' || | ||
|
@@ -405,6 +409,12 @@ function withPersistentBlockChange( reducer ) { | |
? ! markNextChangeAsNotPersistent | ||
: ! isUpdatingSameBlockAttribute( action, lastAction ), | ||
}; | ||
if ( | ||
action.type === 'UPDATE_BLOCK_ATTRIBUTES' && | ||
action.rootClientId | ||
) { | ||
nextState.persistentChangeRootClientId = action.rootClientId; | ||
} | ||
|
||
// In comparing against the previous action, consider only those which | ||
// would have qualified as one which would have been ignored or not | ||
|
@@ -525,8 +535,18 @@ const withBlockReset = ( reducer ) => ( state, action ) => { | |
...mapBlockParents( action.blocks ), | ||
}, | ||
cache: { | ||
...omit( state.cache, visibleClientIds ), | ||
...mapValues( flattenBlocks( action.blocks ), () => ( {} ) ), | ||
...state.cache, | ||
Comment on lines
-528
to
+534
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious why these were being There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was just to keep it simpler. Do you see an issue with this @aduth, @youknowriad? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has changed quite a bit since last I recall being familiar with it, but my concerns would be one of (a) are we risking to leave orphaned child blocks or (b) are we not cleaning up blocks which are no longer part of state. Reading the documentation for the function, it seems like ideally we would want to avoid a merge altogether, and that the only reason we have it is to account for reusable blocks. I'm not familiar if we're to the point with recent changes like #14367 to be able to remove this and let There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That would be ideal, who would be a good person to ask? |
||
...mapValues( | ||
pickBy( | ||
flattenBlocks( action.blocks ), | ||
( block ) => | ||
! isShallowEqual( block, { | ||
...state.byClientId[ block.clientId ], | ||
attributes: state.attributes[ block.clientId ], | ||
} ) | ||
), | ||
() => ( {} ) | ||
), | ||
}, | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a block implements its own save that returns null, this logic will fail, right?
I also wonder about back compatibility: Imagining a current block that uses InnerBlocks without any save and listens to save actions (e.g: executes when saved state changes) to save its InnerBlocks using a rest API. Would a block like that break with this change because if its InnerBlocks are changed the post would not become dirty? I think the odds of a block like that existing are low, but at least we would need to create a dev note.
As an alternative solution, what if we use a supports flag to indicate this type of blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for both, I don't see value in supporting either, though.
It would work, but it would be adding more cruft.
I am not sure. Thoughts @youknowriad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i understand properly where we're coming from here. i believe all these issues are happening because we mix the blocks of the sub-entity into the blocks of the top entity.
I think we discussed this previously, IMO, we should find a way to stop doing this as it doesn't make sense to mix content from entity A into an edit of entity B.
One potential idea I shared previously is to have a unique block list in the "editor" store and each entity has its own "blocks" edits in the "core" store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue is a bit more general than that.
We save serialized content yet detect changes on block objects. Any edit to a block will be dirtying even if its serialization doesn't change.
So like reusable blocks, but resolved into a single blocklist? What do you see as the benefits of that approach over this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefits is that an entity is dirty only if its block list is dirty (A template block list won't include the blocks from the template area)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same is true in this PR.
So for your suggestion, we would need a new property in block objects that potentially contain a selector that gets a block list for their inner blocks. The editor store would then dynamically resolve a complete block list. However, when the block editor makes an edit, it will mutate the full block list, how would the editor know to not dirty the top-level entity. It seems like we are trading some complexity for another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed no silver bullet here. I think "onChange" of the "controlled" inner blocks might be the place where we dispatch the change to two block lists. Conceptually speaking, I feel like having in an entity of core-data, things that are saved in another entity will just bite us with this kind of hidden bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll still have the same issues when we nest one of these blocks inside another.
I don't see a conceptual issue with it. They are object references, it's not like the data is duplicated. That's why this PR was so simple to do.