Skip to content
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

Prevent RESET_BLOCKS from affecting reusable blocks #11746

Merged
merged 4 commits into from
Nov 19, 2018

Conversation

noisysocks
Copy link
Member

Fixes #5754.

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

  1. Create a new post
  2. Create or insert a reusable block
  3. Switch to Text Mode
  4. Make an edit, e.g. add a line of text
  5. Return to Visual Mode
  6. 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.
@noisysocks 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 noisysocks added this to the 4.4 milestone Nov 12, 2018
@noisysocks noisysocks requested a review from aduth November 12, 2018 06:36
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
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
Copy link
Member Author

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@noisysocks noisysocks Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🙂

Copy link
Member

@aduth aduth Nov 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018
@gziolo gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Nov 19, 2018
if ( state && action.type === 'RESET_BLOCKS' ) {
const visibleClientIds = getNestedBlockClientIds( state.order );
return {
...state,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😄

@aduth aduth merged commit 88112df into master Nov 19, 2018
@aduth aduth deleted the fix/disappearing-reusable-blocks branch November 19, 2018 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editing post in Text Mode causes shared blocks to go away
4 participants