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.

Sign up for GitHub

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: Consider RECEIVE_BLOCKS as non-persistent change #14108

Merged
merged 2 commits into from
Feb 27, 2019
Merged

Block Editor: Consider RECEIVE_BLOCKS as non-persistent change #14108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8a9de2
Block Editor: Consider RECEIVE_BLOCKS as non-persistent change
aduth Feb 25, 2019
b24696f
Block Editor: Compare last action in reducer enhancer only if non-ign…
aduth Feb 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 38 additions & 6 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
keys,
isEqual,
isEmpty,
get,
} from 'lodash';

/**
Expand Down Expand Up @@ -187,20 +188,51 @@ export function isUpdatingSameBlockAttribute( action, lastAction ) {
function withPersistentBlockChange( reducer ) {
let lastAction;

/**
* Set of action types for which a blocks state change should be considered
* non-persistent.
*
* @type {Set}
*/
const IGNORED_ACTION_TYPES = new Set( [
'RECEIVE_BLOCKS',
] );

return ( state, action ) => {
let nextState = reducer( state, action );

const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT';

if ( state !== nextState || isExplicitPersistentChange ) {
nextState = {
// Defer to previous state value (or default) unless changing or
// explicitly marking as persistent.
if ( state === nextState && ! isExplicitPersistentChange ) {
return {
...nextState,
isPersistentChange: get( state, [ 'isPersistentChange' ], true ),
};
}

// Some state changes should not be considered persistent, namely those
// which are not a direct result of user interaction.
const isIgnoredActionType = IGNORED_ACTION_TYPES.has( action.type );
if ( isIgnoredActionType ) {
return {
...nextState,
isPersistentChange: (
isExplicitPersistentChange ||
! isUpdatingSameBlockAttribute( action, lastAction )
),
isPersistentChange: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason, we explicitely set this to false when the action is ignored? I'd have expected the same return value as the previous "if" or maybe just return nextState

Copy link
Member Author

Choose a reason for hiding this comment

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

The thought is that since state has changed, we want to explicitly communicate that the change is not one which should be considered as persistent. As I understand it, this is key to what's causing the issue in the first place, where RECEIVE_BLOCKS is incurring a change in the blocks state, and without also explicitly marking it as false, it will be surfaced up through onChange as a persistent change.

gutenberg/packages/block-editor/src/components/provider/index.js

Lines 108 to 122 in 05570ff

if (
newBlocks !== blocks ||
// This happens when a previous input is explicitely marked as persistent.
( newIsPersistent && ! isPersistent )
) {
blocks = newBlocks;
isPersistent = newIsPersistent;
this.isSyncingOutcomingValue = true;
if ( isPersistent ) {
onChange( blocks );
} else {
onInput( blocks );
}
}

I'm fairly convinced this is what's causing the major instability with E2E tests at the moment, where this undo level is created because Puppeteer is fast enough to start typing within a paragraph before the reusable blocks fetch request completes (where the completion of the fetch request triggers RECEIVE_BLOCKS and in turn creates an undo level).

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍

};
}

nextState = {
...nextState,
isPersistentChange: (
isExplicitPersistentChange ||
! isUpdatingSameBlockAttribute( action, lastAction )
),
};

// In comparing against the previous action, consider only those which
// would have qualified as one which would have been ignored or not
// have resulted in a changed state.
lastAction = action;

return nextState;
Expand Down
86 changes: 83 additions & 3 deletions packages/block-editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,12 @@ describe( 'state', () => {
} );

describe( 'isPersistentChange', () => {
it( 'should default a changing state to true', () => {
const state = deepFreeze( blocks( undefined, {} ) );

expect( state.isPersistentChange ).toBe( true );
} );

it( 'should consider any non-exempt block change as persistent', () => {
const original = deepFreeze( blocks( undefined, {
type: 'RESET_BLOCKS',
Expand All @@ -1063,31 +1069,105 @@ describe( 'state', () => {
expect( state.isPersistentChange ).toBe( true );
} );

it( 'should consider any non-exempt block change as persistent across unchanging actions', () => {
let original = deepFreeze( blocks( undefined, {
type: 'RESET_BLOCKS',
blocks: [ {
clientId: 'kumquat',
attributes: {},
innerBlocks: [],
} ],
} ) );
original = blocks( original, {
type: 'NOOP',
} );
original = blocks( original, {
// While RECEIVE_BLOCKS changes state, it's considered
// as ignored, confirmed by this test.
type: 'RECEIVE_BLOCKS',
blocks: [],
} );

const state = blocks( original, {
type: 'UPDATE_BLOCK_ATTRIBUTES',
clientId: 'kumquat',
attributes: {
updated: false,
},
} );

expect( state.isPersistentChange ).toBe( true );
} );

it( 'should consider same block attribute update as exempt', () => {
const original = deepFreeze( blocks( undefined, {
let original = deepFreeze( blocks( undefined, {
type: 'RESET_BLOCKS',
blocks: [ {
clientId: 'kumquat',
attributes: {},
innerBlocks: [],
} ],
} ) );
let state = blocks( original, {
original = blocks( original, {
type: 'UPDATE_BLOCK_ATTRIBUTES',
clientId: 'kumquat',
attributes: {
updated: false,
},
} );

state = blocks( state, {
const state = blocks( original, {
type: 'UPDATE_BLOCK_ATTRIBUTES',
clientId: 'kumquat',
attributes: {
updated: true,
},
} );

expect( state.isPersistentChange ).toBe( false );
} );

it( 'should flag an explicitly marked persistent change', () => {
let original = deepFreeze( blocks( undefined, {
type: 'RESET_BLOCKS',
blocks: [ {
clientId: 'kumquat',
attributes: {},
innerBlocks: [],
} ],
} ) );
original = blocks( original, {
type: 'UPDATE_BLOCK_ATTRIBUTES',
clientId: 'kumquat',
attributes: {
updated: false,
},
} );
original = blocks( original, {
type: 'UPDATE_BLOCK_ATTRIBUTES',
clientId: 'kumquat',
attributes: {
updated: true,
},
} );

const state = blocks( original, {
type: 'MARK_LAST_CHANGE_AS_PERSISTENT',
} );

expect( state.isPersistentChange ).toBe( true );
} );

it( 'should not consider received blocks as persistent change', () => {
const state = blocks( undefined, {
type: 'RECEIVE_BLOCKS',
blocks: [ {
clientId: 'kumquat',
attributes: {},
innerBlocks: [],
} ],
} );

expect( state.isPersistentChange ).toBe( false );
} );
} );
Expand Down
1 change: 0 additions & 1 deletion packages/editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ export const editor = flow( [
withHistory( {
resetTypes: [ 'SETUP_EDITOR_STATE' ],
ignoreTypes: [
'RECEIVE_BLOCKS',
'RESET_POST',
'UPDATE_POST',
],
Expand Down