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

Previews: avoid unneeded block selectors #60543

Merged
merged 4 commits into from
Apr 8, 2024
Merged

Previews: avoid unneeded block selectors #60543

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19a31a1
Preview: avoid unneeded block selectors
ellatrix Apr 7, 2024
04c36b8
Fill in public API
ellatrix Apr 8, 2024
7f32e73
Fill in mode
ellatrix Apr 8, 2024
e3ee74e
Avoid default values in selected object
ellatrix Apr 8, 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
52 changes: 34 additions & 18 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 @@ -562,38 +562,62 @@ function BlockListBlockProvider( props ) {
hasBlockSupport: _hasBlockSupport,
getActiveBlockVariation,
} = select( blocksStore );
const _isSelected = isBlockSelected( clientId );
const canRemove = canRemoveBlock( clientId, rootClientId );
const canMove = canMoveBlock( clientId, rootClientId );
const attributes = getBlockAttributes( clientId );
const { name: blockName, isValid } = blockWithoutAttributes;
const blockType = getBlockType( blockName );
const {
outlineMode,
supportsLayout,
__unstableIsPreviewMode: isPreviewMode,
} = getSettings();
const hasLightBlockWrapper = blockType?.apiVersion > 1;
const previewContext = {
isSelected: false,
blockWithoutAttributes,
name: blockName,
attributes,
isValid,
themeSupportsLayout: supportsLayout,
index: getBlockIndex( clientId ),
isReusable: isReusableBlock( blockType ),
className: hasLightBlockWrapper
? attributes.className
: undefined,
defaultClassName: hasLightBlockWrapper
? getBlockDefaultClassName( blockName )
: undefined,
blockTitle: blockType?.title,
};

// When in preview mode, we can avoid a lot of selection and
// editing related selectors.
if ( isPreviewMode ) {
return previewContext;
}

const _isSelected = isBlockSelected( clientId );
const canRemove = canRemoveBlock( clientId, rootClientId );
const canMove = canMoveBlock( clientId, rootClientId );
const match = getActiveBlockVariation( blockName, attributes );
const { outlineMode, supportsLayout } = getSettings();
const isMultiSelected = isBlockMultiSelected( clientId );
const checkDeep = true;
const isAncestorOfSelectedBlock = hasSelectedInnerBlock(
clientId,
checkDeep
);
const typing = isTyping();
const hasLightBlockWrapper = blockType?.apiVersion > 1;
const movingClientId = hasBlockMovingClientId();
const blockEditingMode = getBlockEditingMode( clientId );

return {
...previewContext,
Copy link
Member Author

@ellatrix ellatrix Apr 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, this makes me wonder what exactly it is below that is making the editable block slower. The pure amount of selectors, or are the particularly slow ones? One of the suspicious ones in getBlockEditingMode because it's recursive, and some of the other selectors make their own calls to getBlockEditingMode.

mode: getBlockMode( clientId ),
isSelectionEnabled: isSelectionEnabled(),
isLocked: !! getTemplateLock( rootClientId ),
templateLock: getTemplateLock( clientId ),
canRemove,
canMove,
blockWithoutAttributes,
name: blockName,
attributes,
isValid,
isSelected: _isSelected,
themeSupportsLayout: supportsLayout,
isTemporarilyEditingAsBlocks:
__unstableGetTemporarilyEditingAsBlocks() === clientId,
blockEditingMode,
Expand All @@ -609,7 +633,6 @@ function BlockListBlockProvider( props ) {
'__experimentalExposeControlsToChildren',
false
) && hasSelectedInnerBlock( clientId ),
index: getBlockIndex( clientId ),
blockApiVersion: blockType?.apiVersion || 1,
blockTitle: match?.title || blockType?.title,
isSubtreeDisabled:
Expand All @@ -629,7 +652,6 @@ function BlockListBlockProvider( props ) {
isMultiSelected &&
! __unstableIsFullySelected() &&
! __unstableSelectionHasUnmergeableBlock(),
isReusable: isReusableBlock( blockType ),
isDragging: isBlockBeingDragged( clientId ),
hasChildSelected: isAncestorOfSelectedBlock,
removeOutline: _isSelected && outlineMode && typing,
Expand All @@ -644,12 +666,6 @@ function BlockListBlockProvider( props ) {
hasEditableOutline:
blockEditingMode !== 'disabled' &&
getBlockEditingMode( rootClientId ) === 'disabled',
className: hasLightBlockWrapper
? attributes.className
: undefined,
defaultClassName: hasLightBlockWrapper
? getBlockDefaultClassName( blockName )
: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this PR doesn't cause more issues. I feel like defensively, we should have default values for all of these keys in the previewContext object no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why all? Only some end up as a public API. The rest is all internal context that we have control over. Fixed the public API ones in 04c36b8.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was more about avoiding breakage. Like code base assuming some keys are defined...

Copy link
Member Author

Choose a reason for hiding this comment

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

I can check all keys in a bit. Most of them are just used once in the block props hook, so it should be easy to double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added one more (mode) since it's a string and also public API. But the rest all end up in a classnames call or falsy checks.

};
},
[ clientId, rootClientId ]
Expand Down
17 changes: 16 additions & 1 deletion packages/block-editor/src/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ export default function BlockList( settings ) {
);
}

const EMPTY_ARRAY = [];
const EMPTY_SET = new Set();

function Items( {
placeholder,
rootClientId,
Expand All @@ -175,6 +178,7 @@ function Items( {
useSelect(
( select ) => {
const {
getSettings,
getBlockOrder,
getSelectedBlockClientId,
getSelectedBlockClientIds,
Expand All @@ -183,9 +187,20 @@ function Items( {
getBlockEditingMode,
__unstableGetEditorMode,
} = select( blockEditorStore );

const _order = getBlockOrder( rootClientId );

if ( getSettings().__unstableIsPreviewMode ) {
return {
order: _order,
selectedBlocks: EMPTY_ARRAY,
visibleBlocks: EMPTY_SET,
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is interesting because it forces all blocks to be async right? This might have a good impact in terms of performance for the different interactions when previews are rendered.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Idea for later)

I actually wonder if we can't introduce a "priority" number in priority-queue at some point where we could say:

  • First render the sync stuff
  • then render async stuff with priority 1
  • and only then (maybe wait a couple async callback before running this list), render async stuff with priority 2

Copy link
Member Author

Choose a reason for hiding this comment

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

They are already async because there is never any selection in previews. What would be priority 2?

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 early return here is just to prevent the selectors from running, especially getBlockEditingMode below.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are already async because there is never any selection in previews. What would be priority 2?

I mean a way to choose that some components even if async are higher priority than other async components.

};
}

const selectedBlockClientId = getSelectedBlockClientId();
return {
order: getBlockOrder( rootClientId ),
order: _order,
selectedBlocks: getSelectedBlockClientIds(),
visibleBlocks: __unstableGetVisibleBlocks(),
shouldRenderAppender:
Expand Down
Loading