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

Jump to bottom

Unmemoize Block component selectors #58355

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Unmemoize Block component selectors #58355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40f773f
Try: unmemoize block selectors
ellatrix Jan 28, 2024
e00b44b
Fix unit tests
ellatrix Jan 28, 2024
f3d63e6
Revert memo removal for selection ids
ellatrix Jan 29, 2024
73e3c78
Adjust mobile too
ellatrix Jan 29, 2024
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
36 changes: 25 additions & 11 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,13 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { memo, useCallback, RawHTML, useContext } from '@wordpress/element';
import {
memo,
useCallback,
RawHTML,
useContext,
useMemo,
} from '@wordpress/element';
import {
getBlockType,
getSaveContent,
Expand Down Expand Up @@ -497,7 +503,8 @@ function BlockListBlockProvider( props ) {
getBlockMode,
isSelectionEnabled,
getTemplateLock,
__unstableGetBlockWithoutInnerBlocks,
getBlockWithoutAttributes,
getBlockAttributes,
canRemoveBlock,
canMoveBlock,

Expand All @@ -524,13 +531,14 @@ function BlockListBlockProvider( props ) {
__unstableGetEditorMode,
getSelectedBlocksInitialCaretPosition,
} = unlock( select( blockEditorStore ) );
const block = __unstableGetBlockWithoutInnerBlocks( clientId );
const blockWithoutAttributes =
getBlockWithoutAttributes( clientId );

// This is a temporary fix.
// This function should never be called when a block is not
// present in the state. It happens now because the order in
// withSelect rendering is not correct.
if ( ! block ) {
if ( ! blockWithoutAttributes ) {
return;
}

Expand All @@ -542,7 +550,8 @@ function BlockListBlockProvider( props ) {
const templateLock = getTemplateLock( rootClientId );
const canRemove = canRemoveBlock( clientId, rootClientId );
const canMove = canMoveBlock( clientId, rootClientId );
const { name: blockName, attributes, isValid } = block;
const attributes = getBlockAttributes( clientId );
const { name: blockName, isValid } = blockWithoutAttributes;
const blockType = getBlockType( blockName );
const match = getActiveBlockVariation( blockName, attributes );
const { outlineMode, supportsLayout } = getSettings();
Expand All @@ -562,11 +571,7 @@ function BlockListBlockProvider( props ) {
isLocked: !! templateLock,
canRemove,
canMove,
// Users of the editor.BlockListBlock filter used to be able to
// access the block prop.
// Ideally these blocks would rely on the clientId prop only.
// This is kept for backward compatibility reasons.
block,
blockWithoutAttributes,
name: blockName,
attributes,
isValid,
Expand Down Expand Up @@ -634,7 +639,7 @@ function BlockListBlockProvider( props ) {
isLocked,
canRemove,
canMove,
block,
blockWithoutAttributes,
name,
attributes,
isValid,
Expand Down Expand Up @@ -665,6 +670,15 @@ function BlockListBlockProvider( props ) {
defaultClassName,
} = selectedProps;

// Users of the editor.BlockListBlock filter used to be able to
// access the block prop.
// Ideally these blocks would rely on the clientId prop only.
// This is kept for backward compatibility reasons.
const block = useMemo(
() => ( { ...blockWithoutAttributes, attributes } ),
[ blockWithoutAttributes, attributes ]
);
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason this is better than selector cache is because it's stored by component instance, not globally.


// Block is sometimes not mounted at the right time, causing it be
// undefined see issue for more info
// https://github.com/WordPress/gutenberg/issues/17013
Expand Down
11 changes: 7 additions & 4 deletions packages/block-editor/src/components/block-list/block.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import { store as blockEditorStore } from '../../store';
import { useLayout } from './layout';
import useScrollUponInsertion from './use-scroll-upon-insertion';
import { useSettings } from '../use-settings';
import { unlock } from '../../lock-unlock';

const EMPTY_ARRAY = [];

Expand Down Expand Up @@ -418,11 +419,13 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId } ) => {
getBlockMode,
isSelectionEnabled,
getTemplateLock,
__unstableGetBlockWithoutInnerBlocks,
getBlockWithoutAttributes,
getBlockAttributes,
canRemoveBlock,
canMoveBlock,
} = select( blockEditorStore );
const block = __unstableGetBlockWithoutInnerBlocks( clientId );
} = unlock( select( blockEditorStore ) );
const block = getBlockWithoutAttributes( clientId );
const attributes = getBlockAttributes( clientId );
const isSelected = isBlockSelected( clientId );
const templateLock = getTemplateLock( rootClientId );
const canRemove = canRemoveBlock( clientId, rootClientId );
Expand All @@ -432,7 +435,7 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId } ) => {
// This function should never be called when a block is not present in
// the state. It happens now because the order in withSelect rendering
// is not correct.
const { name, attributes, isValid } = block || {};
Copy link
Member Author

Choose a reason for hiding this comment

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

For mobile, there's not really a need to keep passing attributes in the block object because mobile is not extensible by third party plugins.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this because I see that the wrapped BlockListBlock component uses the attributes prop several times. For example, when calling getAccessibilityLabel. And that uses, for example, attributes.level to say "this is a level 2 heading".

Copy link
Member Author

Choose a reason for hiding this comment

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

attributes yes, but not block.attributes?

Copy link
Member

Choose a reason for hiding this comment

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

I see, you're assigning the attributes variable elsewhere, with getBlockAttributes :) Nevermind.

const { name, isValid } = block || {};

// Do not add new properties here, use `useSelect` instead to avoid
// leaking new props to the public API (editor.BlockListBlock filter).
Expand Down
34 changes: 15 additions & 19 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ export function getLastInsertedBlocksClientIds( state ) {
return state?.lastBlockInserted?.clientIds;
}

export function getBlockWithoutAttributes( state, clientId ) {
return state.blocks.byClientId.get( clientId );
}

/**
* Returns true if all of the descendants of a block with the given client ID
* have an editing mode of 'disabled', or false otherwise.
Expand All @@ -53,25 +57,17 @@ export function getLastInsertedBlocksClientIds( state ) {
*
* @return {boolean} Whether the block descendants are disabled.
*/
export const isBlockSubtreeDisabled = createSelector(
( state, clientId ) => {
const isChildSubtreeDisabled = ( childClientId ) => {
return (
getBlockEditingMode( state, childClientId ) === 'disabled' &&
getBlockOrder( state, childClientId ).every(
isChildSubtreeDisabled
)
);
};
return getBlockOrder( state, clientId ).every( isChildSubtreeDisabled );
},
( state ) => [
state.blocks.parents,
state.blocks.order,
state.blockEditingModes,
state.blockListSettings,
]
);
export const isBlockSubtreeDisabled = ( state, clientId ) => {
const isChildSubtreeDisabled = ( childClientId ) => {
return (
getBlockEditingMode( state, childClientId ) === 'disabled' &&
getBlockOrder( state, childClientId ).every(
isChildSubtreeDisabled
)
);
};
return getBlockOrder( state, clientId ).every( isChildSubtreeDisabled );
};
Copy link
Member Author

Choose a reason for hiding this comment

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

After #58349, this is no longer called, so the performance improvement of this PR will be lessened.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably still worth to remove the memoization I think... Also the dependencies of this selector weren't fully correct.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this selector should also check the editing mode of the top-level clientId, not just its children.

If I look at the only call in BlockListBlockProvider, I see that it's done anyway, checking blockEditingMode === 'disabled' && isBlockSubtreeDisabled().


/**
* Returns a tree of block objects with only clientID and innerBlocks set.
Expand Down
Loading