Prevent RESET_BLOCKS from affecting reusable blocks #11746

noisysocks · 2018-11-12T06:36:46Z

Changes the editor reducer so that RESET_BLOCKS will only remove blocks that are actually in the post, i.e. blocks that are a descendent of the '' blocks.order key.

This fixes a bug where editing a post in Text Mode would break any reusable blocks in the post.

How to test

Create a new post
Create or insert a reusable block
Switch to Text Mode
Make an edit, e.g. add a line of text
Return to Visual Mode
The reusable block should be unaffected

Changes the editor reducer so that RESET_BLOCKS will only remove blocks that are actually in the post, i.e. blocks that are a descendent of the `''` `blocks.order` key. This fixes a bug where editing a post in Text Mode would break any reusable blocks in the post.

aduth

There's a bit of sadness in that by not continuing with #7453, we're forced to make concessions like these.

aduth · 2018-11-12T14:21:40Z

packages/editor/src/store/reducer.js

+const withBlockReset = ( reducer ) => ( state, action ) => {
+	if ( state && action.type === 'RESET_BLOCKS' ) {
+		const visibleClientIds = getNestedBlockClientIds( state.order );
+		return {


It makes me think, rather than duplicate the logic of the reducer, is there a way we could inject the existing reusable blocks into the action to make sure they're persisted after the reset?

One advantage of this: I'd rather see RESET_BLOCKS remain in the reducer. In fact, I want it to be the only thing there (removing SETUP_EDITOR_STATE instead). See #11641.

Yeah OK. I've pushed up 7d3d116 which changes things so that we instead augment RESET_BLOCKS with a list of client IDs that are referenced by reusable blocks and thus shouldn't be touched.

packages/editor/src/store/reducer.js

Keep the RESET_BLOCKS logic in the reducer by instead using a higher-order reducer to augment RESET_BLOCKS with a list of client IDs that should remain in state as they are referenced by a reusable block.

noisysocks · 2018-11-13T01:04:04Z

There's a bit of sadness in that by not continuing with #7453, we're forced to make concessions like these.

Yeah, I know. But there's a bit of happiness that we will ship 5.0 without this bug! 🙂 I've placed #7119 in the 5.1 milestone and am aiming to get it done while everything is quiet over the holidays.

aduth · 2018-11-14T21:08:50Z

packages/editor/src/store/reducer.js

+const withReusableBlockClientIdsOnReset = ( reducer ) => ( state, action ) => {
+	if ( state && action.type === 'RESET_BLOCKS' ) {
+		const reusableBlockClientIds = map( state.reusableBlocks.data, 'clientId' );
+		action = { ...action, reusableBlockClientIds };


Close, but my expectation is that our reducer doesn't need to change at all, and that our modifications to the action here are to bundle in the reusable blocks we want to preserve into the action.blocks property. Is that possible?

This ties into sadness evoked at #11746 (review) in that ideally at some point we can just drop this higher-order reducer without having to surgically remove it from the reducer.

I wish it could remain an enhancer to the editor reducer specifically, since we have a pattern for those established already. I guess it's not simple enough to detect which of the blocks are reusable from within editor state? Possible, yes, but only with a (bulky) operation of gathering all clientIds from order and doing a difference on those found within byClientId. Since RESET_BLOCKS happens infrequently enough, maybe it's fine to be bulky. I'm not overly concerned about it one way or the other.

Close, but my expectation is that our reducer doesn't need to change at all, and that our modifications to the action here are to bundle in the reusable blocks we want to preserve into the action.blocks property. Is that possible?

That won't work because if we append the reusable blocks that we want to preserve onto action.blocks then the reusable blocks will be added into blocks.order[''] and incorrectly appear in the post.

We need some way of indicating to the blocks.byClientId reducer which blocks should be preserved in state while also indicating to the blocks.order reducer which blocks should not end up in blocks.order['']. This is the role of action.reusableBlockClientIds here.

I wish it could remain an enhancer to the editor reducer specifically, since we have a pattern for those established already. I guess it's not simple enough to detect which of the blocks are reusable from within editor state? Possible, yes, but only with a (bulky) operation of gathering all clientIds from order and doing a difference on those found within byClientId.

A desire to only enhance the editor reducer was what led me to my original approach in 0c92c67 which is similar to what you're describing here. I'd be happy to go back to that approach. One nice thing about it is that it means that the blocks reducer doesn't need to "know" what a reusable block is.

I've reverted 7d3d116 so that we're back at my original approach. I think it makes sense that the enhanced editor reducer as a whole doesn't know or care about reusable blocks—it simply knows that RESET_BLOCKS shouldn't affect any blocks that aren't in blocks.order[''].

I want to stress that regardless of the approach we take here we can revert this entire PR when something like #7453 does eventually land.

Don't be sad, Andrew! 🙂

I want to stress that regardless of the approach we take here we can revert this entire PR when something like #7453 does eventually land.

Don't be sad, Andrew! 🙂

The cynic in me assumes that a future major refactor is never to come 😅

I wonder if a compromise might be to leave the RESET_BLOCKS case in the switch, even if technically dead code, to allow for this to actually be seamlessly dropped.

I wonder if a compromise might be to leave the RESET_BLOCKS case in the switch, even if technically dead code, to allow for this to actually be seamlessly dropped.

Reflecting on this, I'd rather we not have the dead code, and it should become apparent in the expression of the higher-order reducer that the handling of RESET_BLOCKS is to be deferred.

It'll require a bit more care and attention on our part if a refactor should come to pass.

This reverts commit 7d3d116.

aduth · 2018-11-19T16:38:22Z

packages/editor/src/store/reducer.js

+	if ( state && action.type === 'RESET_BLOCKS' ) {
+		const visibleClientIds = getNestedBlockClientIds( state.order );
+		return {
+			...state,


Are we trying to pretend that we don't know there's only the two properties byClientId and order that we're already returning anyways? 😄

noisysocks added [Type] Bug An existing feature does not function as intended [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Nov 12, 2018

noisysocks added this to the 4.4 milestone Nov 12, 2018

noisysocks requested a review from aduth November 12, 2018 06:36

aduth reviewed Nov 12, 2018

View reviewed changes

Augment RESET_BLOCKS action instead of replacing it

7d3d116

Keep the RESET_BLOCKS logic in the reducer by instead using a higher-order reducer to augment RESET_BLOCKS with a list of client IDs that should remain in state as they are referenced by a reusable block.

aduth requested changes Nov 14, 2018

View reviewed changes

noisysocks added 2 commits November 15, 2018 11:17

Revert "Augment RESET_BLOCKS action instead of replacing it"

0593446

This reverts commit 7d3d116.

Use reduce() to simplify getNestedBlockClientIds()

a4ed064

youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Nov 19, 2018

aduth reviewed Nov 19, 2018

View reviewed changes

aduth approved these changes Nov 19, 2018

View reviewed changes

aduth merged commit 88112df into master Nov 19, 2018

aduth deleted the fix/disappearing-reusable-blocks branch November 19, 2018 16:46

noisysocks mentioned this pull request Nov 19, 2018

Reusable Blocks: Reimplement reusable block as embedded editor #7119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent RESET_BLOCKS from affecting reusable blocks #11746

Prevent RESET_BLOCKS from affecting reusable blocks #11746

noisysocks commented Nov 12, 2018

aduth left a comment

aduth Nov 12, 2018

aduth Nov 12, 2018

noisysocks Nov 13, 2018

noisysocks commented Nov 13, 2018

aduth Nov 14, 2018

noisysocks Nov 15, 2018

noisysocks Nov 15, 2018 •

edited

Loading

aduth Nov 19, 2018 •

edited

Loading

aduth Nov 19, 2018

aduth Nov 19, 2018

Prevent RESET_BLOCKS from affecting reusable blocks #11746

Prevent RESET_BLOCKS from affecting reusable blocks #11746

Conversation

noisysocks commented Nov 12, 2018

How to test

aduth left a comment

Choose a reason for hiding this comment

aduth Nov 12, 2018

Choose a reason for hiding this comment

aduth Nov 12, 2018

Choose a reason for hiding this comment

noisysocks Nov 13, 2018

Choose a reason for hiding this comment

noisysocks commented Nov 13, 2018

aduth Nov 14, 2018

Choose a reason for hiding this comment

noisysocks Nov 15, 2018

Choose a reason for hiding this comment

noisysocks Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

aduth Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

aduth Nov 19, 2018

Choose a reason for hiding this comment

aduth Nov 19, 2018

Choose a reason for hiding this comment

noisysocks Nov 15, 2018 •

edited

Loading

aduth Nov 19, 2018 •

edited

Loading