Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multi-entity multi-property undo redo #50911

Merged
merged 9 commits into from
May 29, 2023
Merged

Fix multi-entity multi-property undo redo #50911

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b0a3292
Fix multi-entity multi-property undo redo
youknowriad May 24, 2023
6198c6b
Fix ignored undos
youknowriad May 25, 2023
01ae5b2
Fix unit tests
youknowriad May 25, 2023
7ae67d8
Add more tests
youknowriad May 25, 2023
18733ea
Deprecate getUndoEdit and getRedoEdit
youknowriad May 25, 2023
60db28b
Update the undo/redo format to store diffs (#51002)
youknowriad May 26, 2023
cf1b115
Better types
youknowriad May 29, 2023
0b1bfff
Avoid mutations
youknowriad May 29, 2023
d8da54d
Code style change per review
youknowriad May 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/reference-guides/data/data-core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_
Expand Down Expand Up @@ -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_
Expand Down
6 changes: 3 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/core-data/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_
Expand Down Expand Up @@ -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_
Expand Down
17 changes: 9 additions & 8 deletions packages/core-data/src/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -406,14 +407,14 @@ 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 ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamziel @jsnajdr I've learned today that:

  • you can call private selectors like that in "thunks"
  • you can't (or at least I didn't see any way to do it) call unlock( select ).somePrivateSelector() in "thunks".
  • The first approach is buggy because I had to pass state.root instead of just state (state.root should be just an internal thing, the store author shouldn't even know it exists).

Fixing that bug is going to be a breaking change. Maybe we can still "proxy" .root and deprecate it somehow though. I'm not fixing this in this unrelated PR but just wanted to let you know about this.

Copy link
Member

Choose a reason for hiding this comment

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

Thunks themselves are a private implementation detail of the store. The user of the store doesn't see how a particular action is implemented.

Therefore, the thunk should be able to see the private selectors and actions without having to unlock anything. The select object passed into a thunk should be already "unlocked", it should expose all the private selectors and let you call select.getUndoEdits() directly.

The same applies to private actions and the dispatch object.

Note that the ability to pass functions to select and dispatch, i.e., calling them as select( state => state.foo ) or dispatch( { type: 'BAR' } ), is also unique to thunks -- see how the thunkArgs properties are constructed with Object.assign here. This allows the thunk to access the state directly and dispatch actions to the reducer directly.

If select doesn't expose private selectors, we need to fix the thunkArgs construction: instead of getSelectors() we need to get the "unlocked" selectors.

The first approach is buggy because I had to pass state.root instead of just state

It seems like this was intentional because we're passing store.__unstableOriginalGetState() to select() here. If we didn't want that, we could very easily pass store.getState(). Or was this an oversight that I and @adamziel didn't catch 2 years ago?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If select doesn't expose private selectors, we need to fix the thunkArgs construction: instead of getSelectors() we need to get the "unlocked" selectors.

Yes, I tried without unlocking and it doesn't work, these things are not available.

It seems like this was intentional because we're passing store.__unstableOriginalGetState() to select() here. If we didn't want that, we could very easily pass store.getState(). Or was this an oversight that I and @adamziel didn't catch 2 years ago?

I'd argue that it's oversight because all selectors receive the "state" and not the "original state" with metadata (which is an internal thing added by the data module).

if ( ! undoEdit ) {
return;
}
dispatch( {
type: 'EDIT_ENTITY_RECORD',
...undoEdit,
meta: { isUndo: true },
type: 'UNDO',
stackedEdits: undoEdit,
} );
};

Expand All @@ -424,14 +425,14 @@ 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;
}
dispatch( {
type: 'EDIT_ENTITY_RECORD',
...redoEdit,
meta: { isRedo: true },
type: 'REDO',
stackedEdits: redoEdit,
} );
};

Expand Down
1 change: 0 additions & 1 deletion packages/core-data/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
30 changes: 30 additions & 0 deletions packages/core-data/src/private-selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 > {
Copy link
Member

Choose a reason for hiding this comment

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

We know the return value is an array so Optional< unknown[] > should be better.

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 ];
}
Loading