From 678a1087d8148250432f293d5f7beb1782787217 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Fri, 8 Sep 2023 10:22:04 +0100 Subject: [PATCH 01/10] Extract undo/redo as a separate package --- docs/manifest.json | 6 + package-lock.json | 26 +++ package.json | 1 + packages/core-data/package.json | 1 + packages/core-data/src/actions.js | 49 +++-- packages/core-data/src/private-selectors.ts | 21 +-- packages/core-data/src/reducer.js | 180 +++--------------- packages/core-data/src/selectors.ts | 48 +---- packages/core-data/tsconfig.json | 1 + packages/undo-manager/.npmrc | 1 + packages/undo-manager/CHANGELOG.md | 4 + packages/undo-manager/README.md | 33 ++++ packages/undo-manager/package.json | 37 ++++ packages/undo-manager/src/index.js | 150 +++++++++++++++ packages/undo-manager/src/test/index.js | 196 ++++++++++++++++++++ packages/undo-manager/src/types.ts | 19 ++ packages/undo-manager/tsconfig.json | 10 + tsconfig.json | 1 + 18 files changed, 550 insertions(+), 234 deletions(-) create mode 100644 packages/undo-manager/.npmrc create mode 100644 packages/undo-manager/CHANGELOG.md create mode 100644 packages/undo-manager/README.md create mode 100644 packages/undo-manager/package.json create mode 100644 packages/undo-manager/src/index.js create mode 100644 packages/undo-manager/src/test/index.js create mode 100644 packages/undo-manager/src/types.ts create mode 100644 packages/undo-manager/tsconfig.json diff --git a/docs/manifest.json b/docs/manifest.json index f6643182df7541..3fd25be331bdb9 100644 --- a/docs/manifest.json +++ b/docs/manifest.json @@ -1925,6 +1925,12 @@ "markdown_source": "../packages/token-list/README.md", "parent": "packages" }, + { + "title": "@wordpress/undo-manager", + "slug": "packages-undo-manager", + "markdown_source": "../packages/undo-manager/README.md", + "parent": "packages" + }, { "title": "@wordpress/url", "slug": "packages-url", diff --git a/package-lock.json b/package-lock.json index 16631cc2fa8df9..cac2325c74aa48 100644 --- a/package-lock.json +++ b/package-lock.json @@ -74,6 +74,7 @@ "@wordpress/style-engine": "file:packages/style-engine", "@wordpress/sync": "file:packages/sync", "@wordpress/token-list": "file:packages/token-list", + "@wordpress/undo-manager": "file:packages/undo-manager", "@wordpress/url": "file:packages/url", "@wordpress/viewport": "file:packages/viewport", "@wordpress/warning": "file:packages/warning", @@ -15655,6 +15656,10 @@ "resolved": "packages/token-list", "link": true }, + "node_modules/@wordpress/undo-manager": { + "resolved": "packages/undo-manager", + "link": true + }, "node_modules/@wordpress/url": { "resolved": "packages/url", "link": true @@ -55012,6 +55017,7 @@ "@wordpress/is-shallow-equal": "file:../is-shallow-equal", "@wordpress/private-apis": "file:../private-apis", "@wordpress/sync": "file:../sync", + "@wordpress/undo-manager": "file:../undo-manager", "@wordpress/url": "file:../url", "change-case": "^4.1.2", "equivalent-key-map": "^0.2.2", @@ -56564,6 +56570,18 @@ "node": ">=12" } }, + "packages/undo-manager": { + "name": "@wordpress/undo-manager", + "version": "0.1.0", + "license": "GPL-2.0-or-later", + "dependencies": { + "@babel/runtime": "^7.16.0", + "@wordpress/is-shallow-equal": "file:../is-shallow-equal" + }, + "engines": { + "node": ">=12" + } + }, "packages/url": { "name": "@wordpress/url", "version": "3.42.0", @@ -67896,6 +67914,7 @@ "@wordpress/is-shallow-equal": "file:../is-shallow-equal", "@wordpress/private-apis": "file:../private-apis", "@wordpress/sync": "file:../sync", + "@wordpress/undo-manager": "file:../undo-manager", "@wordpress/url": "file:../url", "change-case": "^4.1.2", "equivalent-key-map": "^0.2.2", @@ -68903,6 +68922,13 @@ "@babel/runtime": "^7.16.0" } }, + "@wordpress/undo-manager": { + "version": "file:packages/undo-manager", + "requires": { + "@babel/runtime": "^7.16.0", + "@wordpress/is-shallow-equal": "file:../is-shallow-equal" + } + }, "@wordpress/url": { "version": "file:packages/url", "requires": { diff --git a/package.json b/package.json index 2bde358171e2a8..706866fc548047 100644 --- a/package.json +++ b/package.json @@ -87,6 +87,7 @@ "@wordpress/sync": "file:packages/sync", "@wordpress/token-list": "file:packages/token-list", "@wordpress/url": "file:packages/url", + "@wordpress/undo-manager": "file:packages/undo-manager", "@wordpress/viewport": "file:packages/viewport", "@wordpress/warning": "file:packages/warning", "@wordpress/widgets": "file:packages/widgets", diff --git a/packages/core-data/package.json b/packages/core-data/package.json index 80bc41ff0a5afe..1f83dc9814400c 100644 --- a/packages/core-data/package.json +++ b/packages/core-data/package.json @@ -43,6 +43,7 @@ "@wordpress/is-shallow-equal": "file:../is-shallow-equal", "@wordpress/private-apis": "file:../private-apis", "@wordpress/sync": "file:../sync", + "@wordpress/undo-manager": "file:../undo-manager", "@wordpress/url": "file:../url", "change-case": "^4.1.2", "equivalent-key-map": "^0.2.2", diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index 1969d2cd717a2a..fde8ced19f5468 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -391,20 +391,29 @@ export const editEntityRecord = edit.edits ); } else { + if ( ! options.undoIngore ) { + select.getUndoManager().record( + [ + { + id: { kind, name, recordId }, + changes: Object.keys( edits ).reduce( + ( acc, key ) => { + acc[ key ] = { + from: editedRecord[ key ], + to: edits[ key ], + }; + return acc; + }, + {} + ), + }, + ], + options.isCached + ); + } dispatch( { type: 'EDIT_ENTITY_RECORD', ...edit, - meta: { - undo: ! options.undoIgnore && { - ...edit, - // Send the current values for things like the first undo stack entry. - edits: Object.keys( edits ).reduce( ( acc, key ) => { - acc[ key ] = editedRecord[ key ]; - return acc; - }, {} ), - isCached: options.isCached, - }, - }, } ); } }; @@ -416,13 +425,14 @@ export const editEntityRecord = export const undo = () => ( { select, dispatch } ) => { - const undoEdit = select.getUndoEdits(); + const undoEdit = select.getUndoManager().getUndoRecord(); if ( ! undoEdit ) { return; } + select.getUndoManager().undo(); dispatch( { type: 'UNDO', - stackedEdits: undoEdit, + record: undoEdit, } ); }; @@ -433,13 +443,14 @@ export const undo = export const redo = () => ( { select, dispatch } ) => { - const redoEdit = select.getRedoEdits(); + const redoEdit = select.getUndoManager().getRedoRecord(); if ( ! redoEdit ) { return; } + select.getUndoManager().redo(); dispatch( { type: 'REDO', - stackedEdits: redoEdit, + record: redoEdit, } ); }; @@ -448,9 +459,11 @@ export const redo = * * @return {Object} Action object. */ -export function __unstableCreateUndoLevel() { - return { type: 'CREATE_UNDO_LEVEL' }; -} +export const __unstableCreateUndoLevel = + () => + ( { select } ) => { + select.getUndoManager().record(); + }; /** * Action triggered to save an entity record. diff --git a/packages/core-data/src/private-selectors.ts b/packages/core-data/src/private-selectors.ts index 1e253b900e1cbb..94aa00e1c8de45 100644 --- a/packages/core-data/src/private-selectors.ts +++ b/packages/core-data/src/private-selectors.ts @@ -1,9 +1,8 @@ /** * Internal dependencies */ -import type { State, UndoEdit } from './selectors'; +import type { State } from './selectors'; -type Optional< T > = T | undefined; type EntityRecordKey = string | number; /** @@ -12,22 +11,10 @@ type EntityRecordKey = string | number; * * @param state State tree. * - * @return The edit. + * @return The undo manager. */ -export function getUndoEdits( state: State ): Optional< UndoEdit[] > { - return state.undo.list[ state.undo.list.length - 1 + 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< UndoEdit[] > { - return state.undo.list[ state.undo.list.length + state.undo.offset ]; +export function getUndoManager( state: State ) { + return state.undoManager; } /** diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index 20755dad4be8d2..41bca666c9dae4 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -8,7 +8,7 @@ import fastDeepEqual from 'fast-deep-equal/es6'; */ import { compose } from '@wordpress/compose'; import { combineReducers } from '@wordpress/data'; -import isShallowEqual from '@wordpress/is-shallow-equal'; +import { createUndoManager } from '@wordpress/undo-manager'; /** * Internal dependencies @@ -185,22 +185,25 @@ export function themeGlobalStyleVariations( state = {}, action ) { const withMultiEntityRecordEdits = ( reducer ) => ( state, action ) => { if ( action.type === 'UNDO' || action.type === 'REDO' ) { - const { stackedEdits } = action; + const { record } = action; let newState = state; - 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, + record.forEach( ( { id: { kind, name, recordId }, changes } ) => { + newState = reducer( newState, { + type: 'EDIT_ENTITY_RECORD', + kind, + name, + recordId, + edits: Object.entries( changes ).reduce( + ( acc, [ key, value ] ) => { + acc[ key ] = + action.type === 'UNDO' ? value.from : value.to; + return acc; }, - } ); - } - ); + {} + ), + } ); + } ); return newState; } @@ -435,151 +438,9 @@ export const entities = ( state = {}, action ) => { }; /** - * @typedef {Object} UndoStateMeta - * - * @property {number} list The undo stack. - * @property {number} offset Where in the undo stack we are. - * @property {Object} cache Cache of unpersisted edits. - */ - -/** @typedef {Array & UndoStateMeta} UndoState */ - -/** - * @type {UndoState} - * - * @todo Given how we use this we might want to make a custom class for it. - */ -const UNDO_INITIAL_STATE = { list: [], offset: 0 }; - -/** - * Reducer keeping track of entity edit undo history. - * - * @param {UndoState} state Current state. - * @param {Object} action Dispatched action. - * - * @return {UndoState} Updated state. + * @type {UndoManager} */ -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 ); - const previousUndoState = nextState.list.pop(); - const updatedUndoState = currentState.cache.reduce( - appendEditToStack, - previousUndoState - ); - nextState.list.push( updatedUndoState ); - - return { - ...nextState, - cache: undefined, - }; - }; - - const appendEditToStack = ( - stack = [], - { kind, name, recordId, property, from, to } - ) => { - const existingEditIndex = stack?.findIndex( - ( { kind: k, name: n, recordId: r, property: p } ) => { - return ( - k === kind && n === name && r === recordId && p === property - ); - } - ); - const nextStack = [ ...stack ]; - if ( existingEditIndex !== -1 ) { - // If the edit is already in the stack leave the initial "from" value. - nextStack[ existingEditIndex ] = { - ...nextStack[ existingEditIndex ], - to, - }; - } else { - nextStack.push( { - kind, - name, - recordId, - property, - from, - to, - } ); - } - return nextStack; - }; - - switch ( action.type ) { - case 'CREATE_UNDO_LEVEL': - return appendCachedEditsToLastUndo( state ); - - case 'UNDO': - case 'REDO': { - const nextState = appendCachedEditsToLastUndo( state ); - return { - ...nextState, - offset: state.offset + ( action.type === 'UNDO' ? -1 : 1 ), - }; - } - - case 'EDIT_ENTITY_RECORD': { - if ( ! action.meta.undo ) { - return state; - } - - 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 ( action.meta.undo.isCached ) { - return { - ...state, - cache: edits.reduce( appendEditToStack, state.cache ), - }; - } - - let nextState = omitPendingRedos( state ); - nextState = appendCachedEditsToLastUndo( nextState ); - nextState = { ...nextState, list: [ ...nextState.list ] }; - // 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( - action.meta.undo.edits - ).filter( ( edit ) => typeof edit !== 'function' ); - const comparisonEdits = Object.values( action.edits ).filter( - ( edit ) => typeof edit !== 'function' - ); - if ( ! isShallowEqual( comparisonUndoEdits, comparisonEdits ) ) { - nextState.list.push( edits ); - } - - return nextState; - } - } - +export function undoManager( state = createUndoManager() ) { return state; } @@ -704,7 +565,8 @@ export default combineReducers( { themeGlobalStyleRevisions, taxonomies, entities, - undo, + //undo, + undoManager, embedPreviews, userPermissions, autosaves, diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 377134ab7c9a3d..1bffcda80b0e9b 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -22,7 +22,7 @@ import { setNestedValue, } from './utils'; import type * as ET from './entity-types'; -import { getUndoEdits, getRedoEdits } from './private-selectors'; +import type { UndoManager } from '@wordpress/undo-manager'; // This is an incomplete, high-level approximation of the State type. // It makes the selectors slightly more safe, but is intended to evolve @@ -40,7 +40,7 @@ export interface State { themeBaseGlobalStyles: Record< string, Object >; themeGlobalStyleVariations: Record< string, string >; themeGlobalStyleRevisions: Record< number, Object >; - undo: UndoState; + undoManager: UndoManager; userPermissions: Record< string, boolean >; users: UserState; navigationFallbackId: EntityRecordKey; @@ -74,20 +74,6 @@ interface EntityConfig { kind: string; } -export interface UndoEdit { - name: string; - kind: string; - recordId: string; - from: any; - to: any; -} - -interface UndoState { - list: Array< UndoEdit[] >; - offset: number; - cache: UndoEdit[]; -} - interface UserState { queries: Record< string, EntityRecordKey[] >; byId: Record< EntityRecordKey, ET.User< 'edit' > >; @@ -875,21 +861,6 @@ export function getLastEntityDeleteError( ?.error; } -/** - * Returns the current undo offset for the - * entity records edits history. The offset - * represents how many items from the end - * of the history stack we are at. 0 is the - * last edit, -1 is the second last, and so on. - * - * @param state State tree. - * - * @return The current undo offset. - */ -function getCurrentUndoOffset( state: State ): number { - return state.undo.offset; -} - /** * Returns the previous edit from the current undo offset * for the entity records edits history, if any. @@ -904,9 +875,7 @@ 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 ]; + return undefined; } /** @@ -923,9 +892,7 @@ 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 ]; + return undefined; } /** @@ -937,7 +904,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( getUndoEdits( state ) ); + return Boolean( state.undoManager.getUndoRecord() ); } /** @@ -949,7 +916,7 @@ export function hasUndo( state: State ): boolean { * @return Whether there is a next edit or not. */ export function hasRedo( state: State ): boolean { - return Boolean( getRedoEdits( state ) ); + return Boolean( state.undoManager.getRedoRecord() ); } /** @@ -1166,7 +1133,8 @@ 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.list.length, state.undo.offset ] + // TODO: fix me, this changes more often than edits. + ( state: State ) => [ state.entities.records ] ); /** diff --git a/packages/core-data/tsconfig.json b/packages/core-data/tsconfig.json index 031d697f8dbe6b..3fe698f758b54d 100644 --- a/packages/core-data/tsconfig.json +++ b/packages/core-data/tsconfig.json @@ -19,6 +19,7 @@ { "path": "../is-shallow-equal" }, { "path": "../private-apis" }, { "path": "../sync" }, + { "path": "../undo-manager" }, { "path": "../url" } ], "include": [ "src/**/*" ] diff --git a/packages/undo-manager/.npmrc b/packages/undo-manager/.npmrc new file mode 100644 index 00000000000000..43c97e719a5a82 --- /dev/null +++ b/packages/undo-manager/.npmrc @@ -0,0 +1 @@ +package-lock=false diff --git a/packages/undo-manager/CHANGELOG.md b/packages/undo-manager/CHANGELOG.md new file mode 100644 index 00000000000000..32b01e3da3a957 --- /dev/null +++ b/packages/undo-manager/CHANGELOG.md @@ -0,0 +1,4 @@ + + +## Unreleased + diff --git a/packages/undo-manager/README.md b/packages/undo-manager/README.md new file mode 100644 index 00000000000000..cc9727469bcadf --- /dev/null +++ b/packages/undo-manager/README.md @@ -0,0 +1,33 @@ +# Undo Manager + +A simple undo manager. + +## Installation + +Install the module + +```bash +npm install @wordpress/undo-manager --save +``` + +## API + + + +### createUndoManager + +Creates an undo manager. + +_Returns_ + +- `UndoManager`: Undo manager. + + + +## Contributing to this package + +This is an individual package that's part of the Gutenberg project. The project is organized as a monorepo. It's made up of multiple self-contained software packages, each with a specific purpose. The packages in this monorepo are published to [npm](https://www.npmjs.com/) and used by [WordPress](https://make.wordpress.org/core/) as well as other software projects. + +To find out more about contributing to this package or Gutenberg as a whole, please read the project's main [contributor guide](https://github.com/WordPress/gutenberg/tree/HEAD/CONTRIBUTING.md). + +

Code is Poetry.

diff --git a/packages/undo-manager/package.json b/packages/undo-manager/package.json new file mode 100644 index 00000000000000..7ca465023b5e83 --- /dev/null +++ b/packages/undo-manager/package.json @@ -0,0 +1,37 @@ +{ + "name": "@wordpress/undo-manager", + "version": "0.1.0", + "description": "A small package to manage undo/redo.", + "author": "The WordPress Contributors", + "license": "GPL-2.0-or-later", + "keywords": [ + "wordpress", + "gutenberg", + "undo", + "history" + ], + "homepage": "https://github.com/WordPress/gutenberg/tree/HEAD/packages/undo-manager/README.md", + "repository": { + "type": "git", + "url": "https://github.com/WordPress/gutenberg.git", + "directory": "packages/undo-manager" + }, + "bugs": { + "url": "https://github.com/WordPress/gutenberg/issues" + }, + "engines": { + "node": ">=12" + }, + "main": "build/index.js", + "module": "build-module/index.js", + "react-native": "src/index", + "types": "build-types", + "sideEffects": false, + "dependencies": { + "@babel/runtime": "^7.16.0", + "@wordpress/is-shallow-equal": "file:../is-shallow-equal" + }, + "publishConfig": { + "access": "public" + } +} diff --git a/packages/undo-manager/src/index.js b/packages/undo-manager/src/index.js new file mode 100644 index 00000000000000..d902f19b38ce2c --- /dev/null +++ b/packages/undo-manager/src/index.js @@ -0,0 +1,150 @@ +/** + * WordPress dependencies + */ +import isShallowEqual from '@wordpress/is-shallow-equal'; + +/** @typedef {import('./types').HistoryRecord} HistoryRecord */ +/** @typedef {import('./types').HistoryChange} HistoryChange */ +/** @typedef {import('./types').HistoryChanges} HistoryChanges */ +/** @typedef {import('./types').UndoManager} UndoManager */ + +/** + * Merge changes for a single item into a record of changes. + * + * @param {Record< string, HistoryChange >} changes1 Previous changes + * @param {Record< string, HistoryChange >} changes2 NextChanges + * + * @return {Record< string, HistoryChange >} Merged changes + */ +function mergeHistoryChanges( changes1, changes2 ) { + /** + * @type {Record< string, HistoryChange >} + */ + const newChanges = { ...changes1 }; + Object.entries( changes2 ).forEach( ( [ key, value ] ) => { + if ( newChanges[ key ] ) { + newChanges[ key ] = { ...newChanges[ key ], to: value.to }; + } else { + newChanges[ key ] = value; + } + } ); + + return newChanges; +} + +/** + * Adds history changes for a single item into a record of changes. + * + * @param {HistoryRecord} record The record to merge into. + * @param {HistoryChanges} changes The changes to merge. + */ +const addHistoryChangesIntoRecord = ( record, changes ) => { + const existingChangesIndex = record?.findIndex( + ( { id: recordIdentifier } ) => { + return typeof recordIdentifier === 'string' + ? recordIdentifier === changes.id + : isShallowEqual( recordIdentifier, changes.id ); + } + ); + const nextRecord = [ ...record ]; + + if ( existingChangesIndex !== -1 ) { + // If the edit is already in the stack leave the initial "from" value. + nextRecord[ existingChangesIndex ] = { + id: changes.id, + changes: mergeHistoryChanges( + nextRecord[ existingChangesIndex ].changes, + changes.changes + ), + }; + } else { + nextRecord.push( changes ); + } + return nextRecord; +}; + +/** + * Creates an undo manager. + * + * @return {UndoManager} Undo manager. + */ +export function createUndoManager() { + /** + * @type {HistoryRecord[]} + */ + let history = []; + /** + * @type {HistoryRecord} + */ + let cachedRecord = []; + /** + * @type {number} + */ + let offset = 0; + + const dropPendingRedos = () => { + history = history.slice( 0, offset || undefined ); + offset = 0; + }; + + const appendCachedRecordToLatestHistoryRecord = () => { + const index = history.length === 0 ? 0 : history.length - 1; + let latestRecord = history[ index ] ?? []; + cachedRecord.forEach( ( changes ) => { + latestRecord = addHistoryChangesIntoRecord( latestRecord, changes ); + } ); + history[ index ] = latestRecord; + }; + + return { + /** + * Record changes into the stiory. + * + * @param {HistoryRecord=} record A record of changes to record. + * @param {boolean} isCached Whether to immediately create an undo point or not. + */ + record( record, isCached = false ) { + const isEmpty = ! record || ! record.length; + if ( isCached ) { + if ( isEmpty ) { + return; + } + record.forEach( ( changes ) => { + cachedRecord = addHistoryChangesIntoRecord( + cachedRecord, + changes + ); + } ); + } else { + dropPendingRedos(); + if ( cachedRecord.length ) { + appendCachedRecordToLatestHistoryRecord(); + } + if ( isEmpty ) { + return; + } + history.push( record ); + } + }, + + undo() { + if ( cachedRecord.length ) { + dropPendingRedos(); + appendCachedRecordToLatestHistoryRecord(); + } + offset -= 1; + }, + + redo() { + offset += 1; + }, + + getUndoRecord() { + return history[ history.length - 1 + offset ]; + }, + + getRedoRecord() { + return history[ history.length + offset ]; + }, + }; +} diff --git a/packages/undo-manager/src/test/index.js b/packages/undo-manager/src/test/index.js new file mode 100644 index 00000000000000..36cc77c6dc27c6 --- /dev/null +++ b/packages/undo-manager/src/test/index.js @@ -0,0 +1,196 @@ +/** + * Internal dependencies + */ +import { createUndoManager } from '../'; + +describe( 'Undo Manager', () => { + it( 'stacks undo levels', () => { + const undo = createUndoManager(); + + undo.record( [ + { id: '1', changes: { value: { from: undefined, to: 1 } } }, + ] ); + expect( undo.getUndoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: undefined, to: 1 } } }, + ] ); + + undo.record( [ { id: '1', changes: { value: { from: 1, to: 2 } } } ] ); + undo.record( [ { id: '1', changes: { value: { from: 2, to: 3 } } } ] ); + expect( undo.getUndoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: 2, to: 3 } } }, + ] ); + } ); + + it( 'handles undos/redos', () => { + const undo = createUndoManager(); + undo.record( [ + { id: '1', changes: { value: { from: undefined, to: 1 } } }, + ] ); + undo.record( [ { id: '1', changes: { value: { from: 1, to: 2 } } } ] ); + undo.record( [ { id: '1', changes: { value: { from: 2, to: 3 } } } ] ); + + undo.undo(); + expect( undo.getUndoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: 1, to: 2 } } }, + ] ); + expect( undo.getRedoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: 2, to: 3 } } }, + ] ); + + undo.undo(); + expect( undo.getUndoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: undefined, to: 1 } } }, + ] ); + expect( undo.getRedoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: 1, to: 2 } } }, + ] ); + + undo.redo(); + undo.redo(); + expect( undo.getUndoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: 2, to: 3 } } }, + ] ); + + undo.record( [ { id: '1', changes: { value: { from: 3, to: 4 } } } ] ); + expect( undo.getUndoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: 3, to: 4 } } }, + ] ); + + // Check that undoing and editing will slice of + // all the levels after the current one. + undo.undo(); + undo.undo(); + undo.record( [ { id: '1', changes: { value: { from: 2, to: 5 } } } ] ); + undo.undo(); + expect( undo.getUndoRecord() ).toEqual( [ + { id: '1', changes: { value: { from: 1, to: 2 } } }, + ] ); + } ); + + it( 'handles cached edits', () => { + const undo = createUndoManager(); + undo.record( [ + { id: '1', changes: { value: { from: undefined, to: 1 } } }, + ] ); + undo.record( + [ { id: '1', changes: { value2: { from: undefined, to: 2 } } } ], + true + ); + undo.record( + [ { id: '1', changes: { value: { from: 1, to: 3 } } } ], + true + ); + undo.record( [ { id: '1', changes: { value: { from: 3, to: 4 } } } ] ); + undo.undo(); + expect( undo.getUndoRecord() ).toEqual( [ + { + id: '1', + changes: { + value: { from: undefined, to: 3 }, + value2: { from: undefined, to: 2 }, + }, + }, + ] ); + } ); + + it( 'handles explicit undo level creation', () => { + const undo = createUndoManager(); + undo.record( [ + { id: '1', changes: { value: { from: undefined, to: 1 } } }, + ] ); + undo.record( [] ); + undo.record(); + undo.undo(); + // Check that nothing happens if there are no pending + // transient edits. + expect( undo.getUndoRecord() ).toBe( undefined ); + undo.redo(); + + // Check that transient edits are merged into the last + // edits. + undo.record( + [ { id: '1', changes: { value2: { from: undefined, to: 2 } } } ], + true + ); + undo.record( [] ); // Records the cached edits. + undo.undo(); + expect( undo.getRedoRecord() ).toEqual( [ + { + id: '1', + changes: { + value: { from: undefined, to: 1 }, + value2: { from: undefined, to: 2 }, + }, + }, + ] ); + } ); + + it( 'explicitly creates an undo level when undoing while there are pending transient edits', () => { + const undo = createUndoManager(); + undo.record( [ + { id: '1', changes: { value: { from: undefined, to: 1 } } }, + ] ); + undo.record( + [ { id: '1', changes: { value2: { from: undefined, to: 2 } } } ], + true + ); + undo.undo(); + expect( undo.getRedoRecord() ).toEqual( [ + { + id: '1', + changes: { + value: { from: undefined, to: 1 }, + value2: { from: undefined, to: 2 }, + }, + }, + ] ); + } ); + + it( 'supports records as ids', () => { + const undo = createUndoManager(); + + undo.record( + [ + { + id: { kind: 'postType', name: 'post', recordId: 1 }, + changes: { value: { from: undefined, to: 1 } }, + }, + ], + true + ); + undo.record( + [ + { + id: { kind: 'postType', name: 'post', recordId: 1 }, + changes: { value2: { from: undefined, to: 2 } }, + }, + ], + true + ); + undo.record( + [ + { + id: { kind: 'postType', name: 'post', recordId: 2 }, + changes: { value: { from: undefined, to: 3 } }, + }, + ], + true + ); + undo.record(); + expect( undo.getUndoRecord() ).toEqual( [ + { + id: { kind: 'postType', name: 'post', recordId: 1 }, + changes: { + value: { from: undefined, to: 1 }, + value2: { from: undefined, to: 2 }, + }, + }, + { + id: { kind: 'postType', name: 'post', recordId: 2 }, + changes: { + value: { from: undefined, to: 3 }, + }, + }, + ] ); + } ); +} ); diff --git a/packages/undo-manager/src/types.ts b/packages/undo-manager/src/types.ts new file mode 100644 index 00000000000000..cbef5388936ed4 --- /dev/null +++ b/packages/undo-manager/src/types.ts @@ -0,0 +1,19 @@ +export type HistoryChange = { + from: any; + to: any; +}; + +export type HistoryChanges = { + id: string | Record< string, any >; + changes: Record< string, HistoryChange >; +}; + +export type HistoryRecord = Array< HistoryChanges >; + +export type UndoManager = { + record: ( record: HistoryRecord, isCached: boolean ) => void; + undo: () => void; + redo: () => void; + getUndoRecord: () => HistoryRecord; + getRedoRecord: () => HistoryRecord; +}; diff --git a/packages/undo-manager/tsconfig.json b/packages/undo-manager/tsconfig.json new file mode 100644 index 00000000000000..e53ddb4792e576 --- /dev/null +++ b/packages/undo-manager/tsconfig.json @@ -0,0 +1,10 @@ +{ + "extends": "../../tsconfig.base.json", + "compilerOptions": { + "rootDir": "src", + "declarationDir": "build-types", + "types": [ "node" ] + }, + "references": [ { "path": "../is-shallow-equal" } ], + "include": [ "src/**/*" ] +} diff --git a/tsconfig.json b/tsconfig.json index 2c395450fb6a0e..4ee1787a247cf7 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -44,6 +44,7 @@ { "path": "packages/style-engine" }, { "path": "packages/sync" }, { "path": "packages/token-list" }, + { "path": "packages/undo-manager" }, { "path": "packages/url" }, { "path": "packages/warning" }, { "path": "packages/wordcount" } From 67f91c8270fad2f412a0d37789ffdfc43d3ef0b0 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Fri, 8 Sep 2023 10:54:25 +0100 Subject: [PATCH 02/10] Fix package.json order --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 706866fc548047..3777de4d958c3a 100644 --- a/package.json +++ b/package.json @@ -86,8 +86,8 @@ "@wordpress/style-engine": "file:packages/style-engine", "@wordpress/sync": "file:packages/sync", "@wordpress/token-list": "file:packages/token-list", - "@wordpress/url": "file:packages/url", "@wordpress/undo-manager": "file:packages/undo-manager", + "@wordpress/url": "file:packages/url", "@wordpress/viewport": "file:packages/viewport", "@wordpress/warning": "file:packages/warning", "@wordpress/widgets": "file:packages/widgets", From c5b39617d3f28bd0ef6884c621cdc93e7bb8b747 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Fri, 8 Sep 2023 11:00:10 +0100 Subject: [PATCH 03/10] Fix unit tests --- docs/reference-guides/data/data-core.md | 2 +- packages/core-data/README.md | 2 +- packages/core-data/src/reducer.js | 12 +- packages/core-data/src/selectors.ts | 9 +- packages/core-data/src/test/reducer.js | 233 ----------------------- packages/core-data/src/test/selectors.js | 54 ------ 6 files changed, 16 insertions(+), 296 deletions(-) diff --git a/docs/reference-guides/data/data-core.md b/docs/reference-guides/data/data-core.md index f2bc3374f9e721..95401834ad4391 100644 --- a/docs/reference-guides/data/data-core.md +++ b/docs/reference-guides/data/data-core.md @@ -371,7 +371,7 @@ _Usage_ _Parameters_ -- _state_ `State`: Editor state. +- _state_ Editor state. _Returns_ diff --git a/packages/core-data/README.md b/packages/core-data/README.md index c778b724149ef3..18e131cd7ab6f1 100644 --- a/packages/core-data/README.md +++ b/packages/core-data/README.md @@ -548,7 +548,7 @@ _Usage_ _Parameters_ -- _state_ `State`: Editor state. +- _state_ Editor state. _Returns_ diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index 41bca666c9dae4..f097d07d047746 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -444,6 +444,16 @@ export function undoManager( state = createUndoManager() ) { return state; } +export function editsReference( state = {}, action ) { + switch ( action.type ) { + case 'EDIT_ENTITY_RECORD': + case 'UNDO': + case 'REDO': + return {}; + } + return state; +} + /** * Reducer managing embed preview data. * @@ -565,7 +575,7 @@ export default combineReducers( { themeGlobalStyleRevisions, taxonomies, entities, - //undo, + editsReference, undoManager, embedPreviews, userPermissions, diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 1bffcda80b0e9b..e4fb2eada0cf87 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -1130,12 +1130,9 @@ export const hasFetchedAutosaves = createRegistrySelector( * * @return A value whose reference will change only when an edit occurs. */ -export const getReferenceByDistinctEdits = createSelector( - // This unused state argument is listed here for the documentation generating tool (docgen). - ( state: State ) => [], - // TODO: fix me, this changes more often than edits. - ( state: State ) => [ state.entities.records ] -); +export function getReferenceByDistinctEdits( state ) { + return state.editsReference; +} /** * Retrieve the frontend template used for a given link. diff --git a/packages/core-data/src/test/reducer.js b/packages/core-data/src/test/reducer.js index 7fac52c33c4b36..4142f65af4c7c4 100644 --- a/packages/core-data/src/test/reducer.js +++ b/packages/core-data/src/test/reducer.js @@ -9,7 +9,6 @@ import deepFreeze from 'deep-freeze'; import { terms, entities, - undo, embedPreviews, userPermissions, autosaves, @@ -142,238 +141,6 @@ describe( 'entities', () => { } ); } ); -describe( 'undo', () => { - let lastValues; - let undoState; - let expectedUndoState; - - const createExpectedDiff = ( property, { from, to } ) => ( { - kind: 'someKind', - name: 'someName', - recordId: 'someRecordId', - property, - from, - to, - } ); - const createNextEditAction = ( edits, isCached ) => { - let action = { - kind: 'someKind', - name: 'someName', - recordId: 'someRecordId', - edits, - }; - action = { - type: 'EDIT_ENTITY_RECORD', - ...action, - meta: { - undo: { - isCached, - edits: lastValues, - }, - }, - }; - lastValues = { ...lastValues, ...edits }; - return action; - }; - const createNextUndoState = ( ...args ) => { - let action = {}; - if ( args[ 0 ] === 'isUndo' || args[ 0 ] === 'isRedo' ) { - // We need to "apply" the undo level here and build - // the action to move the offset. - const lastEdits = - undoState.list[ - 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', - }; - } else if ( args[ 0 ] === 'isCreate' ) { - action = { type: 'CREATE_UNDO_LEVEL' }; - } else if ( args.length ) { - action = createNextEditAction( ...args ); - } - return deepFreeze( undo( undoState, action ) ); - }; - beforeEach( () => { - lastValues = {}; - undoState = undefined; - expectedUndoState = { list: [], offset: 0 }; - } ); - - it( 'initializes', () => { - expect( createNextUndoState() ).toEqual( expectedUndoState ); - } ); - - it( 'stacks undo levels', () => { - undoState = createNextUndoState(); - - // 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( [ - 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( [ - createExpectedDiff( 'value', { from: 1, to: 2 } ), - ] ); - expect( undoState ).toEqual( expectedUndoState ); - undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.list.push( [ - createExpectedDiff( 'value', { from: 2, to: 3 } ), - ] ); - expect( undoState ).toEqual( expectedUndoState ); - } ); - - it( 'stacks multi-property undo levels', () => { - undoState = createNextUndoState(); - - undoState = createNextUndoState( { value: 1 } ); - undoState = createNextUndoState( { value2: 2 } ); - expectedUndoState.list.push( - [ 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.push( [ - createExpectedDiff( 'value', { from: 1, to: 2 } ), - ] ); - expect( undoState ).toEqual( expectedUndoState ); - } ); - - it( 'handles undos/redos', () => { - undoState = createNextUndoState(); - undoState = createNextUndoState( { value: 1 } ); - undoState = createNextUndoState( { value: 2 } ); - undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.list.push( - [ createExpectedDiff( 'value', { from: undefined, to: 1 } ) ], - [ createExpectedDiff( 'value', { from: 1, to: 2 } ) ], - [ createExpectedDiff( 'value', { from: 2, to: 3 } ) ] - ); - expect( undoState ).toEqual( expectedUndoState ); - - // Check that undoing and redoing an equal - // number of steps does not lose edits. - undoState = createNextUndoState( 'isUndo' ); - expectedUndoState.offset--; - expect( undoState ).toEqual( expectedUndoState ); - undoState = createNextUndoState( 'isUndo' ); - expectedUndoState.offset--; - expect( undoState ).toEqual( expectedUndoState ); - undoState = createNextUndoState( 'isRedo' ); - expectedUndoState.offset++; - expect( undoState ).toEqual( expectedUndoState ); - undoState = createNextUndoState( 'isRedo' ); - expectedUndoState.offset++; - expect( undoState ).toEqual( expectedUndoState ); - - // Check that another edit will go on top when there - // is no undo level offset. - undoState = createNextUndoState( { value: 4 } ); - expectedUndoState.list.push( [ - createExpectedDiff( 'value', { from: 3, to: 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.list.pop(); - expectedUndoState.list.pop(); - expectedUndoState.list.push( [ - createExpectedDiff( 'value', { from: 2, to: 5 } ), - ] ); - expect( undoState ).toEqual( expectedUndoState ); - } ); - - it( 'handles flattened undos/redos', () => { - undoState = createNextUndoState(); - undoState = createNextUndoState( { value: 1 } ); - undoState = createNextUndoState( { transientValue: 2 }, true ); - undoState = createNextUndoState( { value: 3 } ); - expectedUndoState.list.push( - [ - createExpectedDiff( 'value', { from: undefined, to: 1 } ), - createExpectedDiff( 'transientValue', { - from: undefined, - to: 2, - } ), - ], - [ createExpectedDiff( 'value', { from: 1, to: 3 } ) ] - ); - expect( undoState ).toEqual( expectedUndoState ); - } ); - - it( 'handles explicit undo level creation', () => { - undoState = createNextUndoState(); - - // Check that nothing happens if there are no pending - // transient edits. - undoState = createNextUndoState( { value: 1 } ); - undoState = createNextUndoState( 'isCreate' ); - expectedUndoState.list.push( [ - createExpectedDiff( 'value', { from: undefined, to: 1 } ), - ] ); - expect( undoState ).toEqual( expectedUndoState ); - - // Check that transient edits are merged into the last - // edits. - undoState = createNextUndoState( { transientValue: 2 }, true ); - undoState = createNextUndoState( 'isCreate' ); - 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( [ - createExpectedDiff( 'value', { from: 1, to: 3 } ), - ] ); - expectedUndoState.offset = -1; - expect( undoState ).toEqual( expectedUndoState ); - } ); - - it( 'explicitly creates an undo level when undoing while there are pending transient edits', () => { - undoState = createNextUndoState(); - undoState = createNextUndoState( { value: 1 } ); - undoState = createNextUndoState( { transientValue: 2 }, true ); - undoState = createNextUndoState( 'isUndo' ); - expectedUndoState.list.push( [ - createExpectedDiff( 'value', { from: undefined, to: 1 } ), - createExpectedDiff( 'transientValue', { from: undefined, to: 2 } ), - ] ); - expectedUndoState.offset--; - expect( undoState ).toEqual( expectedUndoState ); - } ); - - it( 'does not create new levels for the same function edits', () => { - const value = () => {}; - undoState = createNextUndoState(); - undoState = createNextUndoState( { value } ); - undoState = createNextUndoState( { value: () => {} } ); - expect( undoState ).toEqual( expectedUndoState ); - } ); -} ); - describe( 'embedPreviews()', () => { it( 'returns an empty object by default', () => { const state = embedPreviews( undefined, {} ); diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index 84fecc7d07cda9..161d0af4ea5bca 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -22,7 +22,6 @@ import { getAutosave, getAutosaves, getCurrentUser, - getReferenceByDistinctEdits, } from '../selectors'; // getEntityRecord and __experimentalGetEntityRecordNoResolver selectors share the same tests. describe.each( [ @@ -835,56 +834,3 @@ describe( 'getCurrentUser', () => { expect( getCurrentUser( state ) ).toEqual( currentUser ); } ); } ); - -describe( 'getReferenceByDistinctEdits', () => { - it( 'should return referentially equal values across empty states', () => { - const state = { undo: { list: [] } }; - expect( getReferenceByDistinctEdits( state ) ).toBe( - getReferenceByDistinctEdits( state ) - ); - - 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 = { list: [ {} ] }; - const state = { undo: undoStates }; - expect( getReferenceByDistinctEdits( state ) ).toBe( - getReferenceByDistinctEdits( state ) - ); - - const beforeState = { undo: undoStates }; - const afterState = { undo: undoStates }; - expect( getReferenceByDistinctEdits( beforeState ) ).toBe( - getReferenceByDistinctEdits( afterState ) - ); - } ); - - describe( 'when adding edits', () => { - it( 'should return referentially different values across changing states', () => { - const beforeState = { undo: { list: [ {} ] } }; - beforeState.undo.offset = 0; - const afterState = { undo: { list: [ {}, {} ] } }; - afterState.undo.offset = 1; - expect( getReferenceByDistinctEdits( beforeState ) ).not.toBe( - getReferenceByDistinctEdits( afterState ) - ); - } ); - } ); - - describe( 'when using undo', () => { - it( 'should return referentially different values across changing states', () => { - const beforeState = { undo: { list: [ {}, {} ] } }; - beforeState.undo.offset = 1; - const afterState = { undo: { list: [ {}, {} ] } }; - afterState.undo.offset = 0; - expect( getReferenceByDistinctEdits( beforeState ) ).not.toBe( - getReferenceByDistinctEdits( afterState ) - ); - } ); - } ); -} ); From ec71a1b44192d3f2f9325f75e37248812262944e Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Fri, 8 Sep 2023 14:31:45 +0100 Subject: [PATCH 04/10] Small fix --- packages/undo-manager/src/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/undo-manager/src/index.js b/packages/undo-manager/src/index.js index d902f19b38ce2c..b1611df82aea1b 100644 --- a/packages/undo-manager/src/index.js +++ b/packages/undo-manager/src/index.js @@ -93,6 +93,7 @@ export function createUndoManager() { cachedRecord.forEach( ( changes ) => { latestRecord = addHistoryChangesIntoRecord( latestRecord, changes ); } ); + cachedRecord = []; history[ index ] = latestRecord; }; From df4cd0cd4d0e308acc1602d2b3b2cf6c5ba526b1 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Fri, 8 Sep 2023 15:25:30 +0100 Subject: [PATCH 05/10] Fix e2e tests and rename record function --- packages/core-data/src/actions.js | 48 +++++++++++------- packages/undo-manager/src/index.js | 24 +++++++-- packages/undo-manager/src/test/index.js | 66 ++++++++++++++++--------- packages/undo-manager/src/types.ts | 2 +- 4 files changed, 92 insertions(+), 48 deletions(-) diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index fde8ced19f5468..6713c416dc12fb 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -391,25 +391,35 @@ export const editEntityRecord = edit.edits ); } else { - if ( ! options.undoIngore ) { - select.getUndoManager().record( - [ - { - id: { kind, name, recordId }, - changes: Object.keys( edits ).reduce( - ( acc, key ) => { - acc[ key ] = { - from: editedRecord[ key ], - to: edits[ key ], - }; - return acc; - }, - {} - ), - }, - ], - options.isCached - ); + if ( ! options.undoIgnore ) { + let hasChangesToRecord = false; + const changesToRecord = {}; + Object.keys( edits ).forEach( ( key ) => { + const from = editedRecord[ key ]; + const to = edits[ key ]; + if ( + typeof from === 'function' || + typeof from === 'function' + ) { + return; + } + hasChangesToRecord = true; + changesToRecord[ key ] = { + from, + to, + }; + } ); + if ( hasChangesToRecord ) { + select.getUndoManager().addRecord( + [ + { + id: { kind, name, recordId }, + changes: changesToRecord, + }, + ], + options.isCached + ); + } } dispatch( { type: 'EDIT_ENTITY_RECORD', diff --git a/packages/undo-manager/src/index.js b/packages/undo-manager/src/index.js index b1611df82aea1b..96fdba9ff27f30 100644 --- a/packages/undo-manager/src/index.js +++ b/packages/undo-manager/src/index.js @@ -97,6 +97,19 @@ export function createUndoManager() { history[ index ] = latestRecord; }; + /** + * Ignore all changes that don't change values. + * @param {HistoryRecord} record + * @return {HistoryRecord} filtered record. + */ + const filterEqualChanges = ( record ) => { + return record.filter( ( { changes } ) => { + return Object.values( changes ).some( + ( { from, to } ) => ! isShallowEqual( from, to ) + ); + } ); + }; + return { /** * Record changes into the stiory. @@ -104,13 +117,16 @@ export function createUndoManager() { * @param {HistoryRecord=} record A record of changes to record. * @param {boolean} isCached Whether to immediately create an undo point or not. */ - record( record, isCached = false ) { - const isEmpty = ! record || ! record.length; + addRecord( record, isCached = false ) { + const filteredRecord = record + ? filterEqualChanges( record ) + : record; + const isEmpty = ! filteredRecord || ! filteredRecord.length; if ( isCached ) { if ( isEmpty ) { return; } - record.forEach( ( changes ) => { + filteredRecord.forEach( ( changes ) => { cachedRecord = addHistoryChangesIntoRecord( cachedRecord, changes @@ -124,7 +140,7 @@ export function createUndoManager() { if ( isEmpty ) { return; } - history.push( record ); + history.push( filteredRecord ); } }, diff --git a/packages/undo-manager/src/test/index.js b/packages/undo-manager/src/test/index.js index 36cc77c6dc27c6..cb2f73a7739313 100644 --- a/packages/undo-manager/src/test/index.js +++ b/packages/undo-manager/src/test/index.js @@ -7,15 +7,19 @@ describe( 'Undo Manager', () => { it( 'stacks undo levels', () => { const undo = createUndoManager(); - undo.record( [ + undo.addRecord( [ { id: '1', changes: { value: { from: undefined, to: 1 } } }, ] ); expect( undo.getUndoRecord() ).toEqual( [ { id: '1', changes: { value: { from: undefined, to: 1 } } }, ] ); - undo.record( [ { id: '1', changes: { value: { from: 1, to: 2 } } } ] ); - undo.record( [ { id: '1', changes: { value: { from: 2, to: 3 } } } ] ); + undo.addRecord( [ + { id: '1', changes: { value: { from: 1, to: 2 } } }, + ] ); + undo.addRecord( [ + { id: '1', changes: { value: { from: 2, to: 3 } } }, + ] ); expect( undo.getUndoRecord() ).toEqual( [ { id: '1', changes: { value: { from: 2, to: 3 } } }, ] ); @@ -23,11 +27,15 @@ describe( 'Undo Manager', () => { it( 'handles undos/redos', () => { const undo = createUndoManager(); - undo.record( [ + undo.addRecord( [ { id: '1', changes: { value: { from: undefined, to: 1 } } }, ] ); - undo.record( [ { id: '1', changes: { value: { from: 1, to: 2 } } } ] ); - undo.record( [ { id: '1', changes: { value: { from: 2, to: 3 } } } ] ); + undo.addRecord( [ + { id: '1', changes: { value: { from: 1, to: 2 } } }, + ] ); + undo.addRecord( [ + { id: '1', changes: { value: { from: 2, to: 3 } } }, + ] ); undo.undo(); expect( undo.getUndoRecord() ).toEqual( [ @@ -51,7 +59,9 @@ describe( 'Undo Manager', () => { { id: '1', changes: { value: { from: 2, to: 3 } } }, ] ); - undo.record( [ { id: '1', changes: { value: { from: 3, to: 4 } } } ] ); + undo.addRecord( [ + { id: '1', changes: { value: { from: 3, to: 4 } } }, + ] ); expect( undo.getUndoRecord() ).toEqual( [ { id: '1', changes: { value: { from: 3, to: 4 } } }, ] ); @@ -60,7 +70,9 @@ describe( 'Undo Manager', () => { // all the levels after the current one. undo.undo(); undo.undo(); - undo.record( [ { id: '1', changes: { value: { from: 2, to: 5 } } } ] ); + undo.addRecord( [ + { id: '1', changes: { value: { from: 2, to: 5 } } }, + ] ); undo.undo(); expect( undo.getUndoRecord() ).toEqual( [ { id: '1', changes: { value: { from: 1, to: 2 } } }, @@ -69,18 +81,20 @@ describe( 'Undo Manager', () => { it( 'handles cached edits', () => { const undo = createUndoManager(); - undo.record( [ + undo.addRecord( [ { id: '1', changes: { value: { from: undefined, to: 1 } } }, ] ); - undo.record( + undo.addRecord( [ { id: '1', changes: { value2: { from: undefined, to: 2 } } } ], true ); - undo.record( + undo.addRecord( [ { id: '1', changes: { value: { from: 1, to: 3 } } } ], true ); - undo.record( [ { id: '1', changes: { value: { from: 3, to: 4 } } } ] ); + undo.addRecord( [ + { id: '1', changes: { value: { from: 3, to: 4 } } }, + ] ); undo.undo(); expect( undo.getUndoRecord() ).toEqual( [ { @@ -95,24 +109,28 @@ describe( 'Undo Manager', () => { it( 'handles explicit undo level creation', () => { const undo = createUndoManager(); - undo.record( [ + undo.addRecord( [ { id: '1', changes: { value: { from: undefined, to: 1 } } }, ] ); - undo.record( [] ); - undo.record(); - undo.undo(); + // These three calls do nothing because they're empty. + undo.addRecord( [] ); + undo.addRecord(); + undo.addRecord( [ + { id: '1', changes: { value: { from: 1, to: 1 } } }, + ] ); // Check that nothing happens if there are no pending // transient edits. + undo.undo(); expect( undo.getUndoRecord() ).toBe( undefined ); undo.redo(); // Check that transient edits are merged into the last // edits. - undo.record( + undo.addRecord( [ { id: '1', changes: { value2: { from: undefined, to: 2 } } } ], true ); - undo.record( [] ); // Records the cached edits. + undo.addRecord( [] ); // Records the cached edits. undo.undo(); expect( undo.getRedoRecord() ).toEqual( [ { @@ -127,10 +145,10 @@ describe( 'Undo Manager', () => { it( 'explicitly creates an undo level when undoing while there are pending transient edits', () => { const undo = createUndoManager(); - undo.record( [ + undo.addRecord( [ { id: '1', changes: { value: { from: undefined, to: 1 } } }, ] ); - undo.record( + undo.addRecord( [ { id: '1', changes: { value2: { from: undefined, to: 2 } } } ], true ); @@ -149,7 +167,7 @@ describe( 'Undo Manager', () => { it( 'supports records as ids', () => { const undo = createUndoManager(); - undo.record( + undo.addRecord( [ { id: { kind: 'postType', name: 'post', recordId: 1 }, @@ -158,7 +176,7 @@ describe( 'Undo Manager', () => { ], true ); - undo.record( + undo.addRecord( [ { id: { kind: 'postType', name: 'post', recordId: 1 }, @@ -167,7 +185,7 @@ describe( 'Undo Manager', () => { ], true ); - undo.record( + undo.addRecord( [ { id: { kind: 'postType', name: 'post', recordId: 2 }, @@ -176,7 +194,7 @@ describe( 'Undo Manager', () => { ], true ); - undo.record(); + undo.addRecord(); expect( undo.getUndoRecord() ).toEqual( [ { id: { kind: 'postType', name: 'post', recordId: 1 }, diff --git a/packages/undo-manager/src/types.ts b/packages/undo-manager/src/types.ts index cbef5388936ed4..adfd268e17da23 100644 --- a/packages/undo-manager/src/types.ts +++ b/packages/undo-manager/src/types.ts @@ -11,7 +11,7 @@ export type HistoryChanges = { export type HistoryRecord = Array< HistoryChanges >; export type UndoManager = { - record: ( record: HistoryRecord, isCached: boolean ) => void; + addRecord: ( record: HistoryRecord, isCached: boolean ) => void; undo: () => void; redo: () => void; getUndoRecord: () => HistoryRecord; From 62b9f91990916f815d6a175c7ac1990569edd28b Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 11 Sep 2023 10:08:41 +0100 Subject: [PATCH 06/10] Fix more e2e tests --- packages/core-data/src/actions.js | 46 ++++++++++--------------- packages/undo-manager/src/index.js | 26 ++++++++------ packages/undo-manager/src/test/index.js | 43 +++++++++++++++++++++++ 3 files changed, 76 insertions(+), 39 deletions(-) diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index 6713c416dc12fb..cc2a2b9d384d5f 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -392,34 +392,24 @@ export const editEntityRecord = ); } else { if ( ! options.undoIgnore ) { - let hasChangesToRecord = false; - const changesToRecord = {}; - Object.keys( edits ).forEach( ( key ) => { - const from = editedRecord[ key ]; - const to = edits[ key ]; - if ( - typeof from === 'function' || - typeof from === 'function' - ) { - return; - } - hasChangesToRecord = true; - changesToRecord[ key ] = { - from, - to, - }; - } ); - if ( hasChangesToRecord ) { - select.getUndoManager().addRecord( - [ - { - id: { kind, name, recordId }, - changes: changesToRecord, - }, - ], - options.isCached - ); - } + select.getUndoManager().addRecord( + [ + { + id: { kind, name, recordId }, + changes: Object.keys( edits ).reduce( + ( acc, key ) => { + acc[ key ] = { + from: editedRecord[ key ], + to: edits[ key ], + }; + return acc; + }, + {} + ), + }, + ], + options.isCached + ); } dispatch( { type: 'EDIT_ENTITY_RECORD', diff --git a/packages/undo-manager/src/index.js b/packages/undo-manager/src/index.js index 96fdba9ff27f30..6df29cf67e8303 100644 --- a/packages/undo-manager/src/index.js +++ b/packages/undo-manager/src/index.js @@ -98,16 +98,23 @@ export function createUndoManager() { }; /** - * Ignore all changes that don't change values. + * Checks whether a record is empty. + * A record is considered empty if it the changes keep the same values. + * Also updates to function values are ignored. + * * @param {HistoryRecord} record - * @return {HistoryRecord} filtered record. + * @return {boolean} Whether the record is empty. */ - const filterEqualChanges = ( record ) => { - return record.filter( ( { changes } ) => { + const isRecordEmpty = ( record ) => { + const filteredRecord = record.filter( ( { changes } ) => { return Object.values( changes ).some( - ( { from, to } ) => ! isShallowEqual( from, to ) + ( { from, to } ) => + typeof from !== 'function' && + typeof to !== 'function' && + ! isShallowEqual( from, to ) ); } ); + return ! filteredRecord.length; }; return { @@ -118,15 +125,12 @@ export function createUndoManager() { * @param {boolean} isCached Whether to immediately create an undo point or not. */ addRecord( record, isCached = false ) { - const filteredRecord = record - ? filterEqualChanges( record ) - : record; - const isEmpty = ! filteredRecord || ! filteredRecord.length; + const isEmpty = ! record || isRecordEmpty( record ); if ( isCached ) { if ( isEmpty ) { return; } - filteredRecord.forEach( ( changes ) => { + record.forEach( ( changes ) => { cachedRecord = addHistoryChangesIntoRecord( cachedRecord, changes @@ -140,7 +144,7 @@ export function createUndoManager() { if ( isEmpty ) { return; } - history.push( filteredRecord ); + history.push( record ); } }, diff --git a/packages/undo-manager/src/test/index.js b/packages/undo-manager/src/test/index.js index cb2f73a7739313..5e4acd612a5490 100644 --- a/packages/undo-manager/src/test/index.js +++ b/packages/undo-manager/src/test/index.js @@ -211,4 +211,47 @@ describe( 'Undo Manager', () => { }, ] ); } ); + + it( 'should ignore empty records', () => { + const undo = createUndoManager(); + + // All the following changes are considered empty for different reasons. + undo.addRecord(); + undo.addRecord( [] ); + undo.addRecord( [ + { id: '1', changes: { a: { from: 'value', to: 'value' } } }, + ] ); + undo.addRecord( [ + { + id: '1', + changes: { + a: { from: 'value', to: 'value' }, + b: { from: () => {}, to: () => {} }, + }, + }, + ] ); + + expect( undo.getUndoRecord() ).toBeUndefined(); + + // The following changes is not empty + // and should also record the function changes in the history. + + undo.addRecord( [ + { + id: '1', + changes: { + a: { from: 'value1', to: 'value2' }, + b: { from: () => {}, to: () => {} }, + }, + }, + ] ); + + const undoRecord = undo.getUndoRecord(); + expect( undoRecord ).not.toBeUndefined(); + // b is included in the changes. + expect( Object.keys( undoRecord[ 0 ].changes ) ).toEqual( [ + 'a', + 'b', + ] ); + } ); } ); From c992638fbac4d8e82dfcc054cb13e56a615dcf7e Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 11 Sep 2023 11:19:46 +0100 Subject: [PATCH 07/10] Fix typo --- packages/core-data/src/actions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index cc2a2b9d384d5f..a79d1236682582 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -462,7 +462,7 @@ export const redo = export const __unstableCreateUndoLevel = () => ( { select } ) => { - select.getUndoManager().record(); + select.getUndoManager().addRecord(); }; /** From 8618bbe27261674123afd4dcc2b08546a1159aa4 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 11 Sep 2023 11:37:45 +0100 Subject: [PATCH 08/10] Make the undo manager a bundled package --- packages/dependency-extraction-webpack-plugin/lib/util.js | 6 +++++- tools/webpack/packages.js | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/dependency-extraction-webpack-plugin/lib/util.js b/packages/dependency-extraction-webpack-plugin/lib/util.js index a22837af6a72e9..ac637eb33fbb2f 100644 --- a/packages/dependency-extraction-webpack-plugin/lib/util.js +++ b/packages/dependency-extraction-webpack-plugin/lib/util.js @@ -1,5 +1,9 @@ const WORDPRESS_NAMESPACE = '@wordpress/'; -const BUNDLED_PACKAGES = [ '@wordpress/icons', '@wordpress/interface' ]; +const BUNDLED_PACKAGES = [ + '@wordpress/icons', + '@wordpress/interface', + '@wordpress/undo-manager', +]; /** * Default request to global transformation diff --git a/tools/webpack/packages.js b/tools/webpack/packages.js index e5bb74abdb0a1f..3dc8407d7974b9 100644 --- a/tools/webpack/packages.js +++ b/tools/webpack/packages.js @@ -24,7 +24,11 @@ const WORDPRESS_NAMESPACE = '@wordpress/'; // Experimental or other packages that should be private are bundled when used. // That way, we can iterate on these package without making them part of the public API. // See: https://github.com/WordPress/gutenberg/pull/19809 -const BUNDLED_PACKAGES = [ '@wordpress/icons', '@wordpress/interface' ]; +const BUNDLED_PACKAGES = [ + '@wordpress/icons', + '@wordpress/interface', + '@wordpress/undo-manager', +]; // PHP files in packages that have to be copied during build. const bundledPackagesPhpConfig = [ From b6b0f3fc481e73f1196cb33790e0e9534a912c5b Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 11 Sep 2023 11:43:50 +0100 Subject: [PATCH 09/10] Fix typo. Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> --- packages/undo-manager/src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/undo-manager/src/index.js b/packages/undo-manager/src/index.js index 6df29cf67e8303..0a28017242d567 100644 --- a/packages/undo-manager/src/index.js +++ b/packages/undo-manager/src/index.js @@ -119,7 +119,7 @@ export function createUndoManager() { return { /** - * Record changes into the stiory. + * Record changes into the history. * * @param {HistoryRecord=} record A record of changes to record. * @param {boolean} isCached Whether to immediately create an undo point or not. From 3d8ef2e5918c31ec0c7a3d8b0dfc5de10586a14c Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 11 Sep 2023 11:59:56 +0100 Subject: [PATCH 10/10] Rename cached edits to staged edits --- packages/undo-manager/src/index.js | 26 ++++++++++++------------- packages/undo-manager/src/test/index.js | 4 ++-- packages/undo-manager/src/types.ts | 2 +- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/undo-manager/src/index.js b/packages/undo-manager/src/index.js index 0a28017242d567..379172943dfb02 100644 --- a/packages/undo-manager/src/index.js +++ b/packages/undo-manager/src/index.js @@ -76,7 +76,7 @@ export function createUndoManager() { /** * @type {HistoryRecord} */ - let cachedRecord = []; + let stagedRecord = []; /** * @type {number} */ @@ -87,13 +87,13 @@ export function createUndoManager() { offset = 0; }; - const appendCachedRecordToLatestHistoryRecord = () => { + const appendStagedRecordToLatestHistoryRecord = () => { const index = history.length === 0 ? 0 : history.length - 1; let latestRecord = history[ index ] ?? []; - cachedRecord.forEach( ( changes ) => { + stagedRecord.forEach( ( changes ) => { latestRecord = addHistoryChangesIntoRecord( latestRecord, changes ); } ); - cachedRecord = []; + stagedRecord = []; history[ index ] = latestRecord; }; @@ -122,24 +122,24 @@ export function createUndoManager() { * Record changes into the history. * * @param {HistoryRecord=} record A record of changes to record. - * @param {boolean} isCached Whether to immediately create an undo point or not. + * @param {boolean} isStaged Whether to immediately create an undo point or not. */ - addRecord( record, isCached = false ) { + addRecord( record, isStaged = false ) { const isEmpty = ! record || isRecordEmpty( record ); - if ( isCached ) { + if ( isStaged ) { if ( isEmpty ) { return; } record.forEach( ( changes ) => { - cachedRecord = addHistoryChangesIntoRecord( - cachedRecord, + stagedRecord = addHistoryChangesIntoRecord( + stagedRecord, changes ); } ); } else { dropPendingRedos(); - if ( cachedRecord.length ) { - appendCachedRecordToLatestHistoryRecord(); + if ( stagedRecord.length ) { + appendStagedRecordToLatestHistoryRecord(); } if ( isEmpty ) { return; @@ -149,9 +149,9 @@ export function createUndoManager() { }, undo() { - if ( cachedRecord.length ) { + if ( stagedRecord.length ) { dropPendingRedos(); - appendCachedRecordToLatestHistoryRecord(); + appendStagedRecordToLatestHistoryRecord(); } offset -= 1; }, diff --git a/packages/undo-manager/src/test/index.js b/packages/undo-manager/src/test/index.js index 5e4acd612a5490..32ec2713f7bc28 100644 --- a/packages/undo-manager/src/test/index.js +++ b/packages/undo-manager/src/test/index.js @@ -79,7 +79,7 @@ describe( 'Undo Manager', () => { ] ); } ); - it( 'handles cached edits', () => { + it( 'handles staged edits', () => { const undo = createUndoManager(); undo.addRecord( [ { id: '1', changes: { value: { from: undefined, to: 1 } } }, @@ -130,7 +130,7 @@ describe( 'Undo Manager', () => { [ { id: '1', changes: { value2: { from: undefined, to: 2 } } } ], true ); - undo.addRecord( [] ); // Records the cached edits. + undo.addRecord( [] ); // Records the staged edits. undo.undo(); expect( undo.getRedoRecord() ).toEqual( [ { diff --git a/packages/undo-manager/src/types.ts b/packages/undo-manager/src/types.ts index adfd268e17da23..e2e1d995f8e5db 100644 --- a/packages/undo-manager/src/types.ts +++ b/packages/undo-manager/src/types.ts @@ -11,7 +11,7 @@ export type HistoryChanges = { export type HistoryRecord = Array< HistoryChanges >; export type UndoManager = { - addRecord: ( record: HistoryRecord, isCached: boolean ) => void; + addRecord: ( record: HistoryRecord, isStaged: boolean ) => void; undo: () => void; redo: () => void; getUndoRecord: () => HistoryRecord;