Skip to content

getInsertionPoint: Avoid returning a different object on every call #53722

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

Merged
Mamaduka merged 2 commits into from
Aug 16, 2023
Merged

getInsertionPoint: Avoid returning a different object on every call #53722

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2e886b
getInsertionPoint: Avoid returning a different object on every call
Mamaduka Aug 15, 2023
139d7a8
Use consistent return type
Mamaduka Aug 16, 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
3 changes: 1 addition & 2 deletions packages/customize-widgets/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,5 @@ export function isInserterOpened( state ) {
* @return {Object} The root client ID and index to insert at.
*/
export function __experimentalGetInsertionPoint( state ) {
const { rootClientId, insertionIndex } = state.blockInserterPanel;
return { rootClientId, insertionIndex };
Copy link
Member

@jsnajdr jsnajdr Aug 16, 2023

Choose a reason for hiding this comment

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

The reducer state can be false, so the selector now can return also a boolean in addition to the { rootClientId, insertionIndex } object. On one hand that's fine, because false is object-like and false.rootClientId evaluates to undefined. It doesn't crash. On the other hand, the selector's return type becomes messy and TypeScript would be very sad seeing this.

I propose to also patch the reducers so that state.blockInserterPanel is always an object with the same shape, { rootClientId, insertionIndex }, sometimes plus filterValue. The initial values of the fields are null. And actions that until now were setting the state to false would reset the fields back to nulls.

Copy link
Member Author

@Mamaduka Mamaduka Aug 16, 2023

Choose a reason for hiding this comment

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

Agree. This is a bit confusing way to handle the state. I was just trying to keep changes related to the issue here, but I am also happy to work on those improvements.

The same state is responsible for opening the block inserter - setIsInserterOpened( true ). How do you suggest handling a truthy value? The insertionPoint data isn't required for opening the inserter.

Copy link
Member

@jsnajdr jsnajdr Aug 16, 2023

Choose a reason for hiding this comment

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

For the __experimentalGetInsertionPoint selector, it doesn't matter at all whether the state value is true or false. The const { rootClientId } = state destructuring always yields the same undefined value.

But patching the reducers is not as easy as I originally thought, because there's also the isInserterOpened selectors that cares about the boolean value.

We could do this:

const EMPTY_INSERTION_POINT = {
  rootClientId: undefined,
  insertionIndex: undefined,
};
function __experimentalGetInsertionPoint( state ) {
  if ( typeof state == 'boolean' ) {
    return EMPTY_INSERTION_POINT;
  }
  return state;
}

That gives the selector a consistent return type, the same as it had before this PR, and it should be easy to implement.

Copy link
Member Author

@Mamaduka Mamaduka Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good. I'll update the PR.

I think in the future, we could split the state in two - isInserterOpen and blockInsertionPoint.

Copy link
Member Author

@Mamaduka Mamaduka Aug 16, 2023

Choose a reason for hiding this comment

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

Updated in 139d7a8.

return state.blockInserterPanel;
}
4 changes: 1 addition & 3 deletions packages/edit-post/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,9 +472,7 @@ export function isInserterOpened( state ) {
* @return {Object} The root client ID, index to insert at and starting filter value.
*/
export function __experimentalGetInsertionPoint( state ) {
const { rootClientId, insertionIndex, filterValue } =
state.blockInserterPanel;
return { rootClientId, insertionIndex, filterValue };
return state.blockInserterPanel;
}

/**
Expand Down
3 changes: 1 addition & 2 deletions packages/edit-widgets/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,7 @@ export function isInserterOpened( state ) {
* @return {Object} The root client ID and index to insert at.
*/
export function __experimentalGetInsertionPoint( state ) {
const { rootClientId, insertionIndex } = state.blockInserterPanel;
return { rootClientId, insertionIndex };
return state.blockInserterPanel;
}

/**
Expand Down