From b0a3292e3f4791b7b9f79dc7268ecb628dae4f6b Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Wed, 24 May 2023 15:31:21 +0100 Subject: [PATCH 1/9] Fix multi-entity multi-property undo redo --- packages/core-data/src/actions.js | 10 +- packages/core-data/src/reducer.js | 231 +++++++++++++------------ packages/core-data/src/selectors.ts | 18 +- packages/core-data/src/test/reducer.js | 78 ++++----- 4 files changed, 174 insertions(+), 163 deletions(-) diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index ffae417a83cd1..2f4389ad699f1 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -411,9 +411,8 @@ export const undo = return; } dispatch( { - type: 'EDIT_ENTITY_RECORD', - ...undoEdit, - meta: { isUndo: true }, + type: 'UNDO', + stackedEdits: undoEdit, } ); }; @@ -429,9 +428,8 @@ export const redo = return; } dispatch( { - type: 'EDIT_ENTITY_RECORD', - ...redoEdit, - meta: { isRedo: true }, + type: 'REDO', + stackedEdits: redoEdit, } ); }; diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index f04d543919b8c..6363157393159 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -183,6 +183,26 @@ export function themeGlobalStyleVariations( state = {}, action ) { return state; } +const withMultiEntityRecordEdits = ( reducer ) => ( state, action ) => { + if ( action.type === 'UNDO' || action.type === 'REDO' ) { + const { stackedEdits } = action; + + let newState = state; + stackedEdits.forEach( ( { kind, name, recordId, edits } ) => { + newState = reducer( newState, { + type: 'EDIT_ENTITY_RECORD', + kind, + name, + recordId, + edits, + } ); + } ); + return newState; + } + + return reducer( state, action ); +}; + /** * Higher Order Reducer for a given entity config. It supports: * @@ -196,6 +216,8 @@ export function themeGlobalStyleVariations( state = {}, action ) { */ function entity( entityConfig ) { return compose( [ + withMultiEntityRecordEdits, + // Limit to matching action type so we don't attempt to replace action on // an unhandled action. ifMatchingAction( @@ -411,8 +433,9 @@ export const entities = ( state = {}, action ) => { /** * @typedef {Object} UndoStateMeta * - * @property {number} offset Where in the undo stack we are. - * @property {Object} [flattenedUndo] Flattened form of undo stack. + * @property {number} list The undo stack. + * @property {number} offset Where in the undo stack we are. + * @property {Object} cache Cache of unpersisted transient edits. */ /** @typedef {Array & UndoStateMeta} UndoState */ @@ -422,10 +445,7 @@ export const entities = ( state = {}, action ) => { * * @todo Given how we use this we might want to make a custom class for it. */ -const UNDO_INITIAL_STATE = Object.assign( [], { offset: 0 } ); - -/** @type {Object} */ -let lastEditAction; +const UNDO_INITIAL_STATE = { list: [], offset: 0 }; /** * Reducer keeping track of entity edit undo history. @@ -436,107 +456,103 @@ let lastEditAction; * @return {UndoState} Updated state. */ export function undo( state = UNDO_INITIAL_STATE, action ) { + const omitPendingRedos = ( currentState ) => { + return { + ...currentState, + list: currentState.list.slice( + 0, + currentState.offset || undefined + ), + offset: 0, + }; + }; + + const appendCachedEditsToLastUndo = ( currentState ) => { + if ( ! currentState.cache ) { + return currentState; + } + + let nextState = { + ...currentState, + list: [ ...currentState.list ], + }; + nextState = omitPendingRedos( nextState ); + let previousUndoState = nextState.list.pop(); + currentState.cache.forEach( ( edit ) => { + previousUndoState = appendEditsToStack( previousUndoState, edit ); + } ); + nextState.list.push( previousUndoState ); + + return { + ...nextState, + cache: undefined, + }; + }; + + const appendEditsToStack = ( + stack = [], + { kind, name, recordId, edits } + ) => { + const existingEditIndex = stack?.findIndex( + ( { kind: k, name: n, recordId: r } ) => { + return k === kind && n === name && r === recordId; + } + ); + + const nextStack = stack.filter( + ( _, index ) => index !== existingEditIndex + ); + nextStack.push( { + kind, + name, + recordId, + edits: { + ...( existingEditIndex !== -1 + ? stack[ existingEditIndex ].edits + : {} ), + ...edits, + }, + } ); + return nextStack; + }; + switch ( action.type ) { - case 'EDIT_ENTITY_RECORD': case 'CREATE_UNDO_LEVEL': - let isCreateUndoLevel = action.type === 'CREATE_UNDO_LEVEL'; - const isUndoOrRedo = - ! isCreateUndoLevel && - ( action.meta.isUndo || action.meta.isRedo ); - if ( isCreateUndoLevel ) { - action = lastEditAction; - } else if ( ! isUndoOrRedo ) { - // Don't lose the last edit cache if the new one only has transient edits. - // Transient edits don't create new levels so updating the cache would make - // us skip an edit later when creating levels explicitly. - if ( - Object.keys( action.edits ).some( - ( key ) => ! action.transientEdits[ key ] - ) - ) { - lastEditAction = action; - } else { - lastEditAction = { - ...action, - edits: { - ...( lastEditAction && lastEditAction.edits ), - ...action.edits, - }, - }; - } - } + return appendCachedEditsToLastUndo( state ); - /** @type {UndoState} */ - let nextState; - - if ( isUndoOrRedo ) { - // @ts-ignore we might consider using Object.assign({}, state) - nextState = [ ...state ]; - nextState.offset = - state.offset + ( action.meta.isUndo ? -1 : 1 ); - - if ( state.flattenedUndo ) { - // The first undo in a sequence of undos might happen while we have - // flattened undos in state. If this is the case, we want execution - // to continue as if we were creating an explicit undo level. This - // will result in an extra undo level being appended with the flattened - // undo values. - // We also have to take into account if the `lastEditAction` had opted out - // of being tracked in undo history, like the action that persists the latest - // content right before saving. In that case we have to update the `lastEditAction` - // to avoid returning early before applying the existing flattened undos. - isCreateUndoLevel = true; - if ( ! lastEditAction.meta.undo ) { - lastEditAction.meta.undo = { - edits: {}, - }; - } - action = lastEditAction; - } else { - return nextState; - } - } + case 'UNDO': + case 'REDO': { + const nextState = appendCachedEditsToLastUndo( state ); + return { + ...nextState, + offset: state.offset + ( action.type === 'UNDO' ? -1 : 1 ), + }; + } - if ( ! action.meta.undo ) { - return state; - } + case 'EDIT_ENTITY_RECORD': + const isCachedChange = Object.keys( action.edits ).every( + ( key ) => action.transientEdits[ key ] + ); - // Transient edits don't create an undo level, but are - // reachable in the next meaningful edit to which they - // are merged. They are defined in the entity's config. - if ( - ! isCreateUndoLevel && - ! Object.keys( action.edits ).some( - ( key ) => ! action.transientEdits[ key ] - ) - ) { - // @ts-ignore we might consider using Object.assign({}, state) - nextState = [ ...state ]; - nextState.flattenedUndo = { - ...state.flattenedUndo, - ...action.edits, + if ( isCachedChange ) { + return { + ...state, + cache: appendEditsToStack( state.cache, action ), }; - nextState.offset = state.offset; - return nextState; } - // Clear potential redos, because this only supports linear history. - nextState = - // @ts-ignore this needs additional cleanup, probably involving code-level changes - nextState || state.slice( 0, state.offset || undefined ); - nextState.offset = nextState.offset || 0; - nextState.pop(); - if ( ! isCreateUndoLevel ) { - nextState.push( { - kind: action.meta.undo.kind, - name: action.meta.undo.name, - recordId: action.meta.undo.recordId, - edits: { - ...state.flattenedUndo, - ...action.meta.undo.edits, - }, - } ); - } + let nextState = omitPendingRedos( state ); + nextState = appendCachedEditsToLastUndo( nextState ); + nextState = { ...nextState, list: [ ...nextState.list ] }; + const previousUndoState = nextState.list.pop(); + nextState.list.push( + appendEditsToStack( previousUndoState, { + kind: action.kind, + name: action.name, + recordId: action.recordId, + edits: action.meta.undo.edits, + } ) + ); // When an edit is a function it's an optimization to avoid running some expensive operation. // We can't rely on the function references being the same so we opt out of comparing them here. const comparisonUndoEdits = Object.values( @@ -546,15 +562,16 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { ( edit ) => typeof edit !== 'function' ); if ( ! isShallowEqual( comparisonUndoEdits, comparisonEdits ) ) { - nextState.push( { - kind: action.kind, - name: action.name, - recordId: action.recordId, - edits: isCreateUndoLevel - ? { ...state.flattenedUndo, ...action.edits } - : action.edits, - } ); + nextState.list.push( [ + { + kind: action.kind, + name: action.name, + recordId: action.recordId, + edits: action.edits, + }, + ] ); } + return nextState; } diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 7513d91810967..2959d827cc12f 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -73,8 +73,8 @@ interface EntityConfig { kind: string; } -interface UndoState extends Array< Object > { - flattenedUndo: unknown; +interface UndoState { + list: Array< Object >; offset: number; } @@ -889,7 +889,9 @@ function getCurrentUndoOffset( state: State ): number { * @return The edit. */ export function getUndoEdit( state: State ): Optional< any > { - return state.undo[ state.undo.length - 2 + getCurrentUndoOffset( state ) ]; + return state.undo.list[ + state.undo.list.length - 2 + getCurrentUndoOffset( state ) + ]; } /** @@ -901,7 +903,9 @@ export function getUndoEdit( state: State ): Optional< any > { * @return The edit. */ export function getRedoEdit( state: State ): Optional< any > { - return state.undo[ state.undo.length + getCurrentUndoOffset( state ) ]; + return state.undo.list[ + state.undo.list.length + getCurrentUndoOffset( state ) + ]; } /** @@ -1142,11 +1146,7 @@ export const hasFetchedAutosaves = createRegistrySelector( export const getReferenceByDistinctEdits = createSelector( // This unused state argument is listed here for the documentation generating tool (docgen). ( state: State ) => [], - ( state: State ) => [ - state.undo.length, - state.undo.offset, - state.undo.flattenedUndo, - ] + ( state: State ) => [ state.undo.list.length, state.undo.offset ] ); /** diff --git a/packages/core-data/src/test/reducer.js b/packages/core-data/src/test/reducer.js index 63caf5fb83b17..0f43266dcc5df 100644 --- a/packages/core-data/src/test/reducer.js +++ b/packages/core-data/src/test/reducer.js @@ -173,16 +173,13 @@ describe( 'undo', () => { // We need to "apply" the undo level here and build // the action to move the offset. lastEdits = - undoState[ - undoState.length + + undoState.list[ + undoState.list.length + undoState.offset - ( args[ 0 ] === 'isUndo' ? 2 : 0 ) - ].edits; + ][ 0 ].edits; action = { - type: 'EDIT_ENTITY_RECORD', - meta: { - [ args[ 0 ] ]: true, - }, + type: args[ 0 ] === 'isUndo' ? 'UNDO' : 'REDO', }; } else if ( args[ 0 ] === 'isCreate' ) { action = { type: 'CREATE_UNDO_LEVEL' }; @@ -194,8 +191,7 @@ describe( 'undo', () => { beforeEach( () => { lastEdits = {}; undoState = undefined; - expectedUndoState = []; - expectedUndoState.offset = 0; + expectedUndoState = { list: [], offset: 0 }; } ); it( 'initializes', () => { @@ -208,19 +204,19 @@ describe( 'undo', () => { // Check that the first edit creates an undo level for the current state and // one for the new one. undoState = createNextUndoState( { value: 1 } ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); // Check that the second and third edits just create an undo level for // themselves. undoState = createNextUndoState( { value: 2 } ); - expectedUndoState.push( createEditActionPart( { value: 2 } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value: 2 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.push( createEditActionPart( { value: 3 } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value: 3 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -229,11 +225,11 @@ describe( 'undo', () => { undoState = createNextUndoState( { value: 1 } ); undoState = createNextUndoState( { value: 2 } ); undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1 } ), - createEditActionPart( { value: 2 } ), - createEditActionPart( { value: 3 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1 } ) ], + [ createEditActionPart( { value: 2 } ) ], + [ createEditActionPart( { value: 3 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); @@ -255,17 +251,18 @@ describe( 'undo', () => { // Check that another edit will go on top when there // is no undo level offset. undoState = createNextUndoState( { value: 4 } ); - expectedUndoState.push( createEditActionPart( { value: 4 } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value: 4 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); // Check that undoing and editing will slice of // all the levels after the current one. undoState = createNextUndoState( 'isUndo' ); undoState = createNextUndoState( 'isUndo' ); + undoState = createNextUndoState( { value: 5 } ); - expectedUndoState.pop(); - expectedUndoState.pop(); - expectedUndoState.push( createEditActionPart( { value: 5 } ) ); + expectedUndoState.list.pop(); + expectedUndoState.list.pop(); + expectedUndoState.list.push( [ createEditActionPart( { value: 5 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -277,10 +274,10 @@ describe( 'undo', () => { { transientValue: true } ); undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1, transientValue: 2 } ), - createEditActionPart( { value: 3 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1, transientValue: 2 } ) ], + [ createEditActionPart( { value: 3 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -292,9 +289,9 @@ describe( 'undo', () => { // transient edits. undoState = createNextUndoState( { value: 1 } ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); @@ -305,18 +302,17 @@ describe( 'undo', () => { { transientValue: true } ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState[ - expectedUndoState.length - 1 - ].edits.transientValue = 2; + expectedUndoState.list[ + expectedUndoState.list.length - 1 + ][ 0 ].edits.transientValue = 2; expect( undoState ).toEqual( expectedUndoState ); - // Check that undo levels are created with the latest action, - // even if undone. + // Check that create after undo does nothing. undoState = createNextUndoState( { value: 3 } ); undoState = createNextUndoState( 'isUndo' ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState.pop(); - expectedUndoState.push( createEditActionPart( { value: 3 } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value: 3 } ) ] ); + expectedUndoState.offset = -1; expect( undoState ).toEqual( expectedUndoState ); } ); @@ -328,9 +324,9 @@ describe( 'undo', () => { { transientValue: true } ); undoState = createNextUndoState( 'isUndo' ); - expectedUndoState.push( - createEditActionPart( {} ), - createEditActionPart( { value: 1, transientValue: 2 } ) + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1, transientValue: 2 } ) ] ); expectedUndoState.offset--; expect( undoState ).toEqual( expectedUndoState ); @@ -341,7 +337,7 @@ describe( 'undo', () => { undoState = createNextUndoState(); undoState = createNextUndoState( { value } ); undoState = createNextUndoState( { value: () => {} } ); - expectedUndoState.push( createEditActionPart( { value } ) ); + expectedUndoState.list.push( [ createEditActionPart( { value } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); } ); From 6198c6b97466394e2eaa69da3dc15782e39c3570 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Thu, 25 May 2023 04:51:14 +0100 Subject: [PATCH 2/9] Fix ignored undos --- packages/core-data/src/reducer.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index 6363157393159..c2056aa5a2f8a 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -530,6 +530,13 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { } case 'EDIT_ENTITY_RECORD': + // When undo is not specified, + // It's unclear whether we should ignore the undo entirely + // or consider it a transient (cached) edit. + if ( ! action?.meta?.undo ) { + return state; + } + const isCachedChange = Object.keys( action.edits ).every( ( key ) => action.transientEdits[ key ] ); From 01ae5b2632b4f0ec5f35b9da71f5460a2a702f99 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Thu, 25 May 2023 05:41:07 +0100 Subject: [PATCH 3/9] Fix unit tests --- packages/core-data/src/test/selectors.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index 0ea9e26e50543..84fecc7d07cda 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -838,20 +838,20 @@ describe( 'getCurrentUser', () => { describe( 'getReferenceByDistinctEdits', () => { it( 'should return referentially equal values across empty states', () => { - const state = { undo: [] }; + const state = { undo: { list: [] } }; expect( getReferenceByDistinctEdits( state ) ).toBe( getReferenceByDistinctEdits( state ) ); - const beforeState = { undo: [] }; - const afterState = { undo: [] }; + const beforeState = { undo: { list: [] } }; + const afterState = { undo: { list: [] } }; expect( getReferenceByDistinctEdits( beforeState ) ).toBe( getReferenceByDistinctEdits( afterState ) ); } ); it( 'should return referentially equal values across unchanging non-empty state', () => { - const undoStates = [ {} ]; + const undoStates = { list: [ {} ] }; const state = { undo: undoStates }; expect( getReferenceByDistinctEdits( state ) ).toBe( getReferenceByDistinctEdits( state ) @@ -866,9 +866,9 @@ describe( 'getReferenceByDistinctEdits', () => { describe( 'when adding edits', () => { it( 'should return referentially different values across changing states', () => { - const beforeState = { undo: [ {} ] }; + const beforeState = { undo: { list: [ {} ] } }; beforeState.undo.offset = 0; - const afterState = { undo: [ {}, {} ] }; + const afterState = { undo: { list: [ {}, {} ] } }; afterState.undo.offset = 1; expect( getReferenceByDistinctEdits( beforeState ) ).not.toBe( getReferenceByDistinctEdits( afterState ) @@ -878,9 +878,9 @@ describe( 'getReferenceByDistinctEdits', () => { describe( 'when using undo', () => { it( 'should return referentially different values across changing states', () => { - const beforeState = { undo: [ {}, {} ] }; + const beforeState = { undo: { list: [ {}, {} ] } }; beforeState.undo.offset = 1; - const afterState = { undo: [ {}, {} ] }; + const afterState = { undo: { list: [ {}, {} ] } }; afterState.undo.offset = 0; expect( getReferenceByDistinctEdits( beforeState ) ).not.toBe( getReferenceByDistinctEdits( afterState ) From 7ae67d88797bcbc765b8c3d8d7a0dcc8daf545da Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Thu, 25 May 2023 06:01:57 +0100 Subject: [PATCH 4/9] Add more tests --- packages/core-data/src/test/reducer.js | 22 ++++++++++++++++ test/e2e/specs/editor/various/undo.spec.js | 29 ++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/packages/core-data/src/test/reducer.js b/packages/core-data/src/test/reducer.js index 0f43266dcc5df..358cf2d6853da 100644 --- a/packages/core-data/src/test/reducer.js +++ b/packages/core-data/src/test/reducer.js @@ -220,6 +220,28 @@ describe( 'undo', () => { expect( undoState ).toEqual( expectedUndoState ); } ); + it( 'stacks and merges multi-property undo levels', () => { + undoState = createNextUndoState(); + + undoState = createNextUndoState( { value: 1 } ); + undoState = createNextUndoState( { value2: 2 } ); + expectedUndoState.list.push( + [ createEditActionPart( {} ) ], + [ createEditActionPart( { value: 1, value2: undefined } ) ], + [ createEditActionPart( { value2: 2 } ) ] + ); + expect( undoState ).toEqual( expectedUndoState ); + + // Check that that creating another undo level merges the "edits" + undoState = createNextUndoState( { value: 2 } ); + expectedUndoState.list.pop(); // Edits the last list item and pushes a new one + expectedUndoState.list.push( + [ createEditActionPart( { value2: 2, value: 1 } ) ], + [ createEditActionPart( { value: 2 } ) ] + ); + expect( undoState ).toEqual( expectedUndoState ); + } ); + it( 'handles undos/redos', () => { undoState = createNextUndoState(); undoState = createNextUndoState( { value: 1 } ); diff --git a/test/e2e/specs/editor/various/undo.spec.js b/test/e2e/specs/editor/various/undo.spec.js index 29b34ea416ff2..a4b68e1036dcf 100644 --- a/test/e2e/specs/editor/various/undo.spec.js +++ b/test/e2e/specs/editor/various/undo.spec.js @@ -455,6 +455,35 @@ test.describe( 'undo', () => { }, ] ); } ); + + // @see https://github.com/WordPress/gutenberg/issues/12075 + test( 'should be able to undo and redo property cross property changes', async ( { + page, + pageUtils, + } ) => { + await page.getByRole( 'textbox', { name: 'Add title' } ).type( 'a' ); // First step. + await page.keyboard.press( 'Backspace' ); // Second step. + await page.getByRole( 'button', { name: 'Add default block' } ).click(); // third step. + + // Title should be empty + await expect( + page.getByRole( 'textbox', { name: 'Add title' } ) + ).toHaveText( '' ); + + // First undo removes the block. + // Second undo restores the title. + await pageUtils.pressKeys( 'primary+z' ); + await pageUtils.pressKeys( 'primary+z' ); + await expect( + page.getByRole( 'textbox', { name: 'Add title' } ) + ).toHaveText( 'a' ); + + // Redoing the "backspace" should clear the title again. + await pageUtils.pressKeys( 'primaryShift+z' ); + await expect( + page.getByRole( 'textbox', { name: 'Add title' } ) + ).toHaveText( '' ); + } ); } ); class UndoUtils { From 18733eacecdea9c0102904dafa950b63770d39a8 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Thu, 25 May 2023 06:31:51 +0100 Subject: [PATCH 5/9] Deprecate getUndoEdit and getRedoEdit --- docs/reference-guides/data/data-core.md | 4 +++ package-lock.json | 6 ++--- packages/core-data/README.md | 4 +++ packages/core-data/src/actions.js | 7 +++-- packages/core-data/src/index.js | 1 - packages/core-data/src/private-selectors.ts | 30 +++++++++++++++++++++ packages/core-data/src/selectors.ts | 23 +++++++++++----- 7 files changed, 63 insertions(+), 12 deletions(-) create mode 100644 packages/core-data/src/private-selectors.ts diff --git a/docs/reference-guides/data/data-core.md b/docs/reference-guides/data/data-core.md index 1ee04e09550e2..8db3a26dd0977 100644 --- a/docs/reference-guides/data/data-core.md +++ b/docs/reference-guides/data/data-core.md @@ -358,6 +358,8 @@ _Returns_ ### getRedoEdit +> **Deprecated** since 6.3 + Returns the next edit from the current undo offset for the entity records edits history, if any. _Parameters_ @@ -401,6 +403,8 @@ _Returns_ ### getUndoEdit +> **Deprecated** since 6.3 + Returns the previous edit from the current undo offset for the entity records edits history, if any. _Parameters_ diff --git a/package-lock.json b/package-lock.json index 5833c451c403b..43903adae1486 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29227,7 +29227,7 @@ "co": { "version": "4.6.0", "resolved": "https://registry.npmjs.org/co/-/co-4.6.0.tgz", - "integrity": "sha1-bqa989hTrlTMuOR7+gvz+QMfsYQ=", + "integrity": "sha512-QVb0dM5HvG+uaxitm8wONl7jltx8dqhfU33DcqtOZcLSVIKSDDLDi7+0LbAKiyI8hD9u42m2YxXSkMGWThaecQ==", "dev": true }, "code-point-at": { @@ -30804,7 +30804,7 @@ "css.escape": { "version": "1.5.1", "resolved": "https://registry.npmjs.org/css.escape/-/css.escape-1.5.1.tgz", - "integrity": "sha512-YUifsXXuknHlUsmlgyY0PKzgPOr7/FjCePfHNt0jxm83wHZi44VDMQ7/fGNkjY3/jV1MC+1CmZbaHzugyeRtpg==", + "integrity": "sha1-QuJ9T6BK4y+TGktNQZH6nN3ul8s=", "dev": true }, "cssesc": { @@ -41365,7 +41365,7 @@ "lz-string": { "version": "1.4.4", "resolved": "https://registry.npmjs.org/lz-string/-/lz-string-1.4.4.tgz", - "integrity": "sha512-0ckx7ZHRPqb0oUm8zNr+90mtf9DQB60H1wMCjBtfi62Kl3a7JbHob6gA2bC+xRvZoOL+1hzUK8jeuEIQE8svEQ==", + "integrity": "sha1-wNjq82BZ9wV5bh40SBHPTEmNOiY=", "dev": true }, "macos-release": { diff --git a/packages/core-data/README.md b/packages/core-data/README.md index dddc3550e03b2..d2ac90a4a5165 100644 --- a/packages/core-data/README.md +++ b/packages/core-data/README.md @@ -535,6 +535,8 @@ _Returns_ ### getRedoEdit +> **Deprecated** since 6.3 + Returns the next edit from the current undo offset for the entity records edits history, if any. _Parameters_ @@ -578,6 +580,8 @@ _Returns_ ### getUndoEdit +> **Deprecated** since 6.3 + Returns the previous edit from the current undo offset for the entity records edits history, if any. _Parameters_ diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index 2f4389ad699f1..cfab95aae9f8f 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -18,6 +18,7 @@ import { receiveItems, removeItems, receiveQueriedItems } from './queried-data'; import { getOrLoadEntitiesConfig, DEFAULT_ENTITY_KEY } from './entities'; import { createBatch } from './batch'; import { STORE_NAME } from './name'; +import { getUndoEdits, getRedoEdits } from './private-selectors'; /** * Returns an action object used in signalling that authors have been received. @@ -406,7 +407,8 @@ export const editEntityRecord = export const undo = () => ( { select, dispatch } ) => { - const undoEdit = select.getUndoEdit(); + // Todo: we shouldn't have to pass "root" here. + const undoEdit = select( ( state ) => getUndoEdits( state.root ) ); if ( ! undoEdit ) { return; } @@ -423,7 +425,8 @@ export const undo = export const redo = () => ( { select, dispatch } ) => { - const redoEdit = select.getRedoEdit(); + // Todo: we shouldn't have to pass "root" here. + const redoEdit = select( ( state ) => getRedoEdits( state.root ) ); if ( ! redoEdit ) { return; } diff --git a/packages/core-data/src/index.js b/packages/core-data/src/index.js index 43fa4a0b3cd07..c2b491fa8c1ea 100644 --- a/packages/core-data/src/index.js +++ b/packages/core-data/src/index.js @@ -62,7 +62,6 @@ const storeConfig = () => ( { * @see https://github.com/WordPress/gutenberg/blob/HEAD/packages/data/README.md#createReduxStore */ export const store = createReduxStore( STORE_NAME, storeConfig() ); - register( store ); export { default as EntityProvider } from './entity-provider'; diff --git a/packages/core-data/src/private-selectors.ts b/packages/core-data/src/private-selectors.ts new file mode 100644 index 0000000000000..508b56e82c4f2 --- /dev/null +++ b/packages/core-data/src/private-selectors.ts @@ -0,0 +1,30 @@ +/** + * Internal dependencies + */ +import type { State } from './selectors'; + +type Optional< T > = T | undefined; + +/** + * Returns the previous edit from the current undo offset + * for the entity records edits history, if any. + * + * @param state State tree. + * + * @return The edit. + */ +export function getUndoEdits( state: State ): Optional< any > { + return state.undo.list[ state.undo.list.length - 2 + state.undo.offset ]; +} + +/** + * Returns the next edit from the current undo offset + * for the entity records edits history, if any. + * + * @param state State tree. + * + * @return The edit. + */ +export function getRedoEdits( state: State ): Optional< any > { + return state.undo.list[ state.undo.list.length + state.undo.offset ]; +} diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 2959d827cc12f..8f6a1767d8b94 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -22,6 +22,7 @@ import { setNestedValue, } from './utils'; import type * as ET from './entity-types'; +import { getUndoEdits, getRedoEdits } from './private-selectors'; // This is an incomplete, high-level approximation of the State type. // It makes the selectors slightly more safe, but is intended to evolve @@ -884,28 +885,38 @@ function getCurrentUndoOffset( state: State ): number { * Returns the previous edit from the current undo offset * for the entity records edits history, if any. * - * @param state State tree. + * @deprecated since 6.3 + * + * @param state State tree. * * @return The edit. */ export function getUndoEdit( state: State ): Optional< any > { + deprecated( "select( 'core' ).getUndoEdit()", { + since: '6.3', + } ); return state.undo.list[ state.undo.list.length - 2 + getCurrentUndoOffset( state ) - ]; + ]?.[ 0 ]; } /** * Returns the next edit from the current undo offset * for the entity records edits history, if any. * - * @param state State tree. + * @deprecated since 6.3 + * + * @param state State tree. * * @return The edit. */ export function getRedoEdit( state: State ): Optional< any > { + deprecated( "select( 'core' ).getRedoEdit()", { + since: '6.3', + } ); return state.undo.list[ state.undo.list.length + getCurrentUndoOffset( state ) - ]; + ]?.[ 0 ]; } /** @@ -917,7 +928,7 @@ export function getRedoEdit( state: State ): Optional< any > { * @return Whether there is a previous edit or not. */ export function hasUndo( state: State ): boolean { - return Boolean( getUndoEdit( state ) ); + return Boolean( getUndoEdits( state ) ); } /** @@ -929,7 +940,7 @@ export function hasUndo( state: State ): boolean { * @return Whether there is a next edit or not. */ export function hasRedo( state: State ): boolean { - return Boolean( getRedoEdit( state ) ); + return Boolean( getRedoEdits( state ) ); } /** From 60db28b201e27a784ecdb1255c6f03331aebf74c Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Fri, 26 May 2023 14:12:24 +0100 Subject: [PATCH 6/9] Update the undo/redo format to store diffs (#51002) --- packages/core-data/src/private-selectors.ts | 2 +- packages/core-data/src/reducer.js | 107 +++++++++--------- packages/core-data/src/test/reducer.js | 115 +++++++++++--------- 3 files changed, 122 insertions(+), 102 deletions(-) diff --git a/packages/core-data/src/private-selectors.ts b/packages/core-data/src/private-selectors.ts index 508b56e82c4f2..178754e3b72b1 100644 --- a/packages/core-data/src/private-selectors.ts +++ b/packages/core-data/src/private-selectors.ts @@ -14,7 +14,7 @@ type Optional< T > = T | undefined; * @return The edit. */ export function getUndoEdits( state: State ): Optional< any > { - return state.undo.list[ state.undo.list.length - 2 + state.undo.offset ]; + return state.undo.list[ state.undo.list.length - 1 + state.undo.offset ]; } /** diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index c2056aa5a2f8a..a12f0be244108 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -188,15 +188,19 @@ const withMultiEntityRecordEdits = ( reducer ) => ( state, action ) => { const { stackedEdits } = action; let newState = state; - stackedEdits.forEach( ( { kind, name, recordId, edits } ) => { - newState = reducer( newState, { - type: 'EDIT_ENTITY_RECORD', - kind, - name, - recordId, - edits, - } ); - } ); + stackedEdits.forEach( + ( { kind, name, recordId, property, from, to } ) => { + newState = reducer( newState, { + type: 'EDIT_ENTITY_RECORD', + kind, + name, + recordId, + edits: { + [ property ]: action.type === 'UNDO' ? from : to, + }, + } ); + } + ); return newState; } @@ -479,7 +483,7 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { nextState = omitPendingRedos( nextState ); let previousUndoState = nextState.list.pop(); currentState.cache.forEach( ( edit ) => { - previousUndoState = appendEditsToStack( previousUndoState, edit ); + previousUndoState = appendEditToStack( previousUndoState, edit ); } ); nextState.list.push( previousUndoState ); @@ -489,30 +493,31 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { }; }; - const appendEditsToStack = ( + const appendEditToStack = ( stack = [], - { kind, name, recordId, edits } + { kind, name, recordId, property, from, to } ) => { const existingEditIndex = stack?.findIndex( - ( { kind: k, name: n, recordId: r } ) => { - return k === kind && n === name && r === recordId; + ( { kind: k, name: n, recordId: r, property: p } ) => { + return ( + k === kind && n === name && r === recordId && p === property + ); } ); - - const nextStack = stack.filter( - ( _, index ) => index !== existingEditIndex - ); - nextStack.push( { - kind, - name, - recordId, - edits: { - ...( existingEditIndex !== -1 - ? stack[ existingEditIndex ].edits - : {} ), - ...edits, - }, - } ); + const nextStack = [ ...stack ]; + if ( existingEditIndex !== -1 ) { + // If the edit is already in the stack leave the initial "from" value. + nextStack[ existingEditIndex ].to = to; + } else { + nextStack.push( { + kind, + name, + recordId, + property, + from, + to, + } ); + } return nextStack; }; @@ -529,11 +534,8 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { }; } - case 'EDIT_ENTITY_RECORD': - // When undo is not specified, - // It's unclear whether we should ignore the undo entirely - // or consider it a transient (cached) edit. - if ( ! action?.meta?.undo ) { + case 'EDIT_ENTITY_RECORD': { + if ( ! action.meta.undo ) { return state; } @@ -541,25 +543,32 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { ( key ) => action.transientEdits[ key ] ); + const edits = Object.keys( action.edits ).map( ( key ) => { + return { + kind: action.kind, + name: action.name, + recordId: action.recordId, + property: key, + from: action.meta.undo.edits[ key ], + to: action.edits[ key ], + }; + } ); + if ( isCachedChange ) { + let newCache = state.cache; + edits.forEach( + ( edit ) => + ( newCache = appendEditToStack( newCache, edit ) ) + ); return { ...state, - cache: appendEditsToStack( state.cache, action ), + cache: newCache, }; } let nextState = omitPendingRedos( state ); nextState = appendCachedEditsToLastUndo( nextState ); nextState = { ...nextState, list: [ ...nextState.list ] }; - const previousUndoState = nextState.list.pop(); - nextState.list.push( - appendEditsToStack( previousUndoState, { - kind: action.kind, - name: action.name, - recordId: action.recordId, - edits: action.meta.undo.edits, - } ) - ); // When an edit is a function it's an optimization to avoid running some expensive operation. // We can't rely on the function references being the same so we opt out of comparing them here. const comparisonUndoEdits = Object.values( @@ -569,17 +578,11 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { ( edit ) => typeof edit !== 'function' ); if ( ! isShallowEqual( comparisonUndoEdits, comparisonEdits ) ) { - nextState.list.push( [ - { - kind: action.kind, - name: action.name, - recordId: action.recordId, - edits: action.edits, - }, - ] ); + nextState.list.push( edits ); } return nextState; + } } return state; diff --git a/packages/core-data/src/test/reducer.js b/packages/core-data/src/test/reducer.js index 358cf2d6853da..4f7d9b9c0d2ae 100644 --- a/packages/core-data/src/test/reducer.js +++ b/packages/core-data/src/test/reducer.js @@ -143,28 +143,34 @@ describe( 'entities', () => { } ); describe( 'undo', () => { - let lastEdits; + let lastValues; let undoState; let expectedUndoState; - const createEditActionPart = ( edits ) => ( { + + const createExpectedDiff = ( property, { from, to } ) => ( { kind: 'someKind', name: 'someName', recordId: 'someRecordId', - edits, + property, + from, + to, } ); const createNextEditAction = ( edits, transientEdits = {} ) => { let action = { - ...createEditActionPart( edits ), + kind: 'someKind', + name: 'someName', + recordId: 'someRecordId', + edits, transientEdits, }; action = { type: 'EDIT_ENTITY_RECORD', ...action, meta: { - undo: { ...action, edits: lastEdits }, + undo: { edits: lastValues }, }, }; - lastEdits = { ...lastEdits, ...edits }; + lastValues = { ...lastValues, ...edits }; return action; }; const createNextUndoState = ( ...args ) => { @@ -172,12 +178,15 @@ describe( 'undo', () => { if ( args[ 0 ] === 'isUndo' || args[ 0 ] === 'isRedo' ) { // We need to "apply" the undo level here and build // the action to move the offset. - lastEdits = + const lastEdits = undoState.list[ - undoState.list.length + - undoState.offset - - ( args[ 0 ] === 'isUndo' ? 2 : 0 ) - ][ 0 ].edits; + undoState.list.length - + ( args[ 0 ] === 'isUndo' ? 1 : 0 ) + + undoState.offset + ]; + lastEdits.forEach( ( { property, from, to } ) => { + lastValues[ property ] = args[ 0 ] === 'isUndo' ? from : to; + } ); action = { type: args[ 0 ] === 'isUndo' ? 'UNDO' : 'REDO', }; @@ -189,7 +198,7 @@ describe( 'undo', () => { return deepFreeze( undo( undoState, action ) ); }; beforeEach( () => { - lastEdits = {}; + lastValues = {}; undoState = undefined; expectedUndoState = { list: [], offset: 0 }; } ); @@ -204,41 +213,41 @@ describe( 'undo', () => { // Check that the first edit creates an undo level for the current state and // one for the new one. undoState = createNextUndoState( { value: 1 } ); - expectedUndoState.list.push( - [ createEditActionPart( {} ) ], - [ createEditActionPart( { value: 1 } ) ] - ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: undefined, to: 1 } ), + ] ); expect( undoState ).toEqual( expectedUndoState ); // Check that the second and third edits just create an undo level for // themselves. undoState = createNextUndoState( { value: 2 } ); - expectedUndoState.list.push( [ createEditActionPart( { value: 2 } ) ] ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: 1, to: 2 } ), + ] ); expect( undoState ).toEqual( expectedUndoState ); undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.list.push( [ createEditActionPart( { value: 3 } ) ] ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: 2, to: 3 } ), + ] ); expect( undoState ).toEqual( expectedUndoState ); } ); - it( 'stacks and merges multi-property undo levels', () => { + it( 'stacks multi-property undo levels', () => { undoState = createNextUndoState(); undoState = createNextUndoState( { value: 1 } ); undoState = createNextUndoState( { value2: 2 } ); expectedUndoState.list.push( - [ createEditActionPart( {} ) ], - [ createEditActionPart( { value: 1, value2: undefined } ) ], - [ createEditActionPart( { value2: 2 } ) ] + [ createExpectedDiff( 'value', { from: undefined, to: 1 } ) ], + [ createExpectedDiff( 'value2', { from: undefined, to: 2 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); // Check that that creating another undo level merges the "edits" undoState = createNextUndoState( { value: 2 } ); - expectedUndoState.list.pop(); // Edits the last list item and pushes a new one - expectedUndoState.list.push( - [ createEditActionPart( { value2: 2, value: 1 } ) ], - [ createEditActionPart( { value: 2 } ) ] - ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: 1, to: 2 } ), + ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -248,10 +257,9 @@ describe( 'undo', () => { undoState = createNextUndoState( { value: 2 } ); undoState = createNextUndoState( { value: 3 } ); expectedUndoState.list.push( - [ createEditActionPart( {} ) ], - [ createEditActionPart( { value: 1 } ) ], - [ createEditActionPart( { value: 2 } ) ], - [ createEditActionPart( { value: 3 } ) ] + [ createExpectedDiff( 'value', { from: undefined, to: 1 } ) ], + [ createExpectedDiff( 'value', { from: 1, to: 2 } ) ], + [ createExpectedDiff( 'value', { from: 2, to: 3 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); @@ -273,7 +281,9 @@ describe( 'undo', () => { // Check that another edit will go on top when there // is no undo level offset. undoState = createNextUndoState( { value: 4 } ); - expectedUndoState.list.push( [ createEditActionPart( { value: 4 } ) ] ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: 3, to: 4 } ), + ] ); expect( undoState ).toEqual( expectedUndoState ); // Check that undoing and editing will slice of @@ -284,7 +294,9 @@ describe( 'undo', () => { undoState = createNextUndoState( { value: 5 } ); expectedUndoState.list.pop(); expectedUndoState.list.pop(); - expectedUndoState.list.push( [ createEditActionPart( { value: 5 } ) ] ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: 2, to: 5 } ), + ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -297,9 +309,14 @@ describe( 'undo', () => { ); undoState = createNextUndoState( { value: 3 } ); expectedUndoState.list.push( - [ createEditActionPart( {} ) ], - [ createEditActionPart( { value: 1, transientValue: 2 } ) ], - [ createEditActionPart( { value: 3 } ) ] + [ + createExpectedDiff( 'value', { from: undefined, to: 1 } ), + createExpectedDiff( 'transientValue', { + from: undefined, + to: 2, + } ), + ], + [ createExpectedDiff( 'value', { from: 1, to: 3 } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); @@ -311,10 +328,9 @@ describe( 'undo', () => { // transient edits. undoState = createNextUndoState( { value: 1 } ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState.list.push( - [ createEditActionPart( {} ) ], - [ createEditActionPart( { value: 1 } ) ] - ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: undefined, to: 1 } ), + ] ); expect( undoState ).toEqual( expectedUndoState ); // Check that transient edits are merged into the last @@ -324,16 +340,18 @@ describe( 'undo', () => { { transientValue: true } ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState.list[ - expectedUndoState.list.length - 1 - ][ 0 ].edits.transientValue = 2; + expectedUndoState.list[ expectedUndoState.list.length - 1 ].push( + createExpectedDiff( 'transientValue', { from: undefined, to: 2 } ) + ); expect( undoState ).toEqual( expectedUndoState ); // Check that create after undo does nothing. undoState = createNextUndoState( { value: 3 } ); undoState = createNextUndoState( 'isUndo' ); undoState = createNextUndoState( 'isCreate' ); - expectedUndoState.list.push( [ createEditActionPart( { value: 3 } ) ] ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: 1, to: 3 } ), + ] ); expectedUndoState.offset = -1; expect( undoState ).toEqual( expectedUndoState ); } ); @@ -346,10 +364,10 @@ describe( 'undo', () => { { transientValue: true } ); undoState = createNextUndoState( 'isUndo' ); - expectedUndoState.list.push( - [ createEditActionPart( {} ) ], - [ createEditActionPart( { value: 1, transientValue: 2 } ) ] - ); + expectedUndoState.list.push( [ + createExpectedDiff( 'value', { from: undefined, to: 1 } ), + createExpectedDiff( 'transientValue', { from: undefined, to: 2 } ), + ] ); expectedUndoState.offset--; expect( undoState ).toEqual( expectedUndoState ); } ); @@ -359,7 +377,6 @@ describe( 'undo', () => { undoState = createNextUndoState(); undoState = createNextUndoState( { value } ); undoState = createNextUndoState( { value: () => {} } ); - expectedUndoState.list.push( [ createEditActionPart( { value } ) ] ); expect( undoState ).toEqual( expectedUndoState ); } ); } ); From cf1b11580a06e205f556c4063c54ad7506437592 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 29 May 2023 08:29:04 +0100 Subject: [PATCH 7/9] Better types --- packages/core-data/src/private-selectors.ts | 6 +++--- packages/core-data/src/selectors.ts | 11 ++++++++++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/core-data/src/private-selectors.ts b/packages/core-data/src/private-selectors.ts index 178754e3b72b1..0ac2c75023969 100644 --- a/packages/core-data/src/private-selectors.ts +++ b/packages/core-data/src/private-selectors.ts @@ -1,7 +1,7 @@ /** * Internal dependencies */ -import type { State } from './selectors'; +import type { State, UndoEdit } from './selectors'; type Optional< T > = T | undefined; @@ -13,7 +13,7 @@ type Optional< T > = T | undefined; * * @return The edit. */ -export function getUndoEdits( state: State ): Optional< any > { +export function getUndoEdits( state: State ): Optional< UndoEdit[] > { return state.undo.list[ state.undo.list.length - 1 + state.undo.offset ]; } @@ -25,6 +25,6 @@ export function getUndoEdits( state: State ): Optional< any > { * * @return The edit. */ -export function getRedoEdits( state: State ): Optional< any > { +export function getRedoEdits( state: State ): Optional< UndoEdit[] > { return state.undo.list[ state.undo.list.length + state.undo.offset ]; } diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 8f6a1767d8b94..a6b7774d37094 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -74,9 +74,18 @@ interface EntityConfig { kind: string; } +export interface UndoEdit { + name: string; + kind: string; + recordId: string; + from: any; + to: any; +} + interface UndoState { - list: Array< Object >; + list: Array< UndoEdit[] >; offset: number; + cache: UndoEdit[]; } interface UserState { From 0b1bfffd36848c86ebf67fe14972708e6590c891 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 29 May 2023 10:42:41 +0100 Subject: [PATCH 8/9] Avoid mutations --- packages/core-data/src/reducer.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index a12f0be244108..beac3205aaa15 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -507,7 +507,10 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { const nextStack = [ ...stack ]; if ( existingEditIndex !== -1 ) { // If the edit is already in the stack leave the initial "from" value. - nextStack[ existingEditIndex ].to = to; + nextStack[ existingEditIndex ] = { + ...nextStack[ existingEditIndex ], + to, + }; } else { nextStack.push( { kind, @@ -555,14 +558,9 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { } ); if ( isCachedChange ) { - let newCache = state.cache; - edits.forEach( - ( edit ) => - ( newCache = appendEditToStack( newCache, edit ) ) - ); return { ...state, - cache: newCache, + cache: edits.reduce( appendEditToStack, state.cache ), }; } From d8da54d983d97701bbdb807e52656b410dc41a65 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 29 May 2023 11:59:16 +0100 Subject: [PATCH 9/9] Code style change per review --- packages/core-data/src/reducer.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index beac3205aaa15..b7dd9d73df15a 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -481,11 +481,12 @@ export function undo( state = UNDO_INITIAL_STATE, action ) { list: [ ...currentState.list ], }; nextState = omitPendingRedos( nextState ); - let previousUndoState = nextState.list.pop(); - currentState.cache.forEach( ( edit ) => { - previousUndoState = appendEditToStack( previousUndoState, edit ); - } ); - nextState.list.push( previousUndoState ); + const previousUndoState = nextState.list.pop(); + const updatedUndoState = currentState.cache.reduce( + appendEditToStack, + previousUndoState + ); + nextState.list.push( updatedUndoState ); return { ...nextState,