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.

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

Performance: optimize the usage of getBlockIndex #13067

Merged
merged 6 commits into from
Jan 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 7 additions & 6 deletions packages/editor/src/components/block-actions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export default compose( [
withSelect( ( select, props ) => {
const {
getBlocksByClientId,
getBlockIndex,
getTemplateLock,
getBlockRootClientId,
} = select( 'core/editor' );
Expand All @@ -45,22 +44,18 @@ export default compose( [
const rootClientId = getBlockRootClientId( props.clientIds[ 0 ] );

return {
firstSelectedIndex: getBlockIndex( first( castArray( props.clientIds ) ), rootClientId ),
lastSelectedIndex: getBlockIndex( last( castArray( props.clientIds ) ), rootClientId ),
isLocked: !! getTemplateLock( rootClientId ),
blocks,
canDuplicate,
rootClientId,
extraProps: props,
};
} ),
withDispatch( ( dispatch, props ) => {
withDispatch( ( dispatch, props, { select } ) => {
Copy link
Member

Choose a reason for hiding this comment

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

@aduth - I don't see it on the list in #12890.

Anyways, I'm happy seeing that select is helping us to improve performance. We still didn't discover any downsides which is encouraging to do more refactoring like this.

Copy link
Member

Choose a reason for hiding this comment

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

@aduth - I don't see it on the list in #12890.

I think it was missed because I was explicitly looking for withDispatch( ( dispatch, ownProps ) or withDispatch( ( dispatch, {. We apparently have more variation 😬

Copy link
Member

Choose a reason for hiding this comment

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

I think it was missed because I was explicitly looking for withDispatch( ( dispatch, ownProps ) or withDispatch( ( dispatch, {. We apparently have more variation 😬

This is the only occurrence of the second argument being named as "props" for withDispatch, and best I could tell (by a regular expression search of withDispatch\( \( dispatch, [^{o]) there are no other variations.

const {
clientIds,
rootClientId,
blocks,
firstSelectedIndex,
lastSelectedIndex,
isLocked,
canDuplicate,
} = props;
Expand All @@ -78,6 +73,8 @@ export default compose( [
return;
}

const { getBlockIndex } = select( 'core/editor' );
const lastSelectedIndex = getBlockIndex( last( castArray( clientIds ) ), rootClientId );
const clonedBlocks = blocks.map( ( block ) => cloneBlock( block ) );
insertBlocks(
clonedBlocks,
Expand All @@ -98,11 +95,15 @@ export default compose( [
},
onInsertBefore() {
if ( ! isLocked ) {
const { getBlockIndex } = select( 'core/editor' );
const firstSelectedIndex = getBlockIndex( first( castArray( clientIds ) ), rootClientId );
insertDefaultBlock( {}, rootClientId, firstSelectedIndex );
}
},
onInsertAfter() {
if ( ! isLocked ) {
const { getBlockIndex } = select( 'core/editor' );
const lastSelectedIndex = getBlockIndex( last( castArray( clientIds ) ), rootClientId );
insertDefaultBlock( {}, rootClientId, lastSelectedIndex + 1 );
}
},
Expand Down
5 changes: 3 additions & 2 deletions packages/editor/src/components/block-draggable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ const BlockDraggable = ( { children, clientId, rootClientId, blockElementId, ind

export default withSelect( ( select, { clientId } ) => {
const { getBlockIndex, getBlockRootClientId } = select( 'core/editor' );
const rootClientId = getBlockRootClientId( clientId );
return {
index: getBlockIndex( clientId ),
rootClientId: getBlockRootClientId( clientId ),
index: getBlockIndex( clientId, rootClientId ),
rootClientId,
};
} )( BlockDraggable );
11 changes: 7 additions & 4 deletions packages/editor/src/components/block-drop-zone/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ class BlockDropZone extends Component {
}

getInsertIndex( position ) {
const { index } = this.props;
if ( index !== undefined ) {
const { clientId, rootClientId, getBlockIndex } = this.props;
if ( clientId !== undefined ) {
const index = getBlockIndex( clientId, rootClientId );
return position.y === 'top' ? index : index + 1;
}
}
Expand All @@ -83,7 +84,7 @@ class BlockDropZone extends Component {
}

onDrop( event, position ) {
const { rootClientId: dstRootClientId, clientId: dstClientId, index: dstIndex, getClientIdsOfDescendants } = this.props;
const { rootClientId: dstRootClientId, clientId: dstClientId, getClientIdsOfDescendants, getBlockIndex } = this.props;
const { srcRootClientId, srcClientId, srcIndex, type } = parseDropEvent( event );

const isBlockDropType = ( dropType ) => dropType === 'block';
Expand All @@ -101,6 +102,7 @@ class BlockDropZone extends Component {
return;
}

const dstIndex = dstClientId ? getBlockIndex( dstClientId, dstRootClientId ) : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

What dst stands for? It would read better if we would avoid such abbreviations.

const positionIndex = this.getInsertIndex( position );
// If the block is kept at the same level and moved downwards,
// subtract to account for blocks shifting upward to occupy its old position.
Expand Down Expand Up @@ -154,10 +156,11 @@ export default compose(
};
} ),
withSelect( ( select, { rootClientId } ) => {
const { getClientIdsOfDescendants, getTemplateLock } = select( 'core/editor' );
const { getClientIdsOfDescendants, getTemplateLock, getBlockIndex } = select( 'core/editor' );
return {
isLocked: !! getTemplateLock( rootClientId ),
getClientIdsOfDescendants,
getBlockIndex,
Copy link
Member

Choose a reason for hiding this comment

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

I assume that it isn't something we can handle using withDispatch and select call.

};
} ),
withFilters( 'editor.BlockDropZone' )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ function BlockListAppender( {
disabled={ disabled }
/>
) }
isAppender
/>
</div>
);
Expand Down
40 changes: 22 additions & 18 deletions packages/editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { KeyboardShortcuts, withFilters } from '@wordpress/components';
import { __, sprintf } from '@wordpress/i18n';
import { withDispatch, withSelect } from '@wordpress/data';
import { withViewportMatch } from '@wordpress/viewport';
import { compose } from '@wordpress/compose';
import { compose, pure } from '@wordpress/compose';

/**
* Internal dependencies
Expand Down Expand Up @@ -59,7 +59,6 @@ export class BlockListBlock extends Component {
this.maybeHover = this.maybeHover.bind( this );
this.forceFocusedContextualToolbar = this.forceFocusedContextualToolbar.bind( this );
this.hideHoverEffects = this.hideHoverEffects.bind( this );
this.insertBlocksAfter = this.insertBlocksAfter.bind( this );
this.onFocus = this.onFocus.bind( this );
this.preventDrag = this.preventDrag.bind( this );
this.onPointerDown = this.onPointerDown.bind( this );
Expand Down Expand Up @@ -233,10 +232,6 @@ export class BlockListBlock extends Component {
}
}

insertBlocksAfter( blocks ) {
this.props.onInsertBlocks( blocks, this.props.order + 1 );
}

/**
* Marks the block as selected when focused and not already selected. This
* specifically handles the case where block does not set focus on its own
Expand Down Expand Up @@ -361,7 +356,6 @@ export class BlockListBlock extends Component {
<HoverArea container={ this.wrapperNode }>
{ ( { hoverArea } ) => {
const {
order,
mode,
isFocusMode,
hasFixedToolbar,
Expand Down Expand Up @@ -397,11 +391,9 @@ export class BlockListBlock extends Component {
// Empty paragraph blocks should always show up as unselected.
const showEmptyBlockSideInserter =
( isSelected || isHovered ) && isEmptyDefaultBlock && isValid;
const showSideInserter =
( isSelected || isHovered ) && isEmptyDefaultBlock;
const shouldAppearSelected =
! isFocusMode &&
! showSideInserter &&
! showEmptyBlockSideInserter &&
isSelected &&
! isTypingWithinBlock;
const shouldAppearHovered =
Expand All @@ -420,7 +412,7 @@ export class BlockListBlock extends Component {
! isFocusMode && isHovered && ! isEmptyDefaultBlock;
const shouldShowContextualToolbar =
! hasFixedToolbar &&
! showSideInserter &&
! showEmptyBlockSideInserter &&
( ( isSelected &&
( ! isTypingWithinBlock || isCaretWithinFormattedText ) ) ||
isFirstMultiSelected );
Expand Down Expand Up @@ -474,7 +466,7 @@ export class BlockListBlock extends Component {
isSelected={ isSelected }
attributes={ attributes }
setAttributes={ this.setAttributes }
insertBlocksAfter={ isLocked ? undefined : this.insertBlocksAfter }
insertBlocksAfter={ isLocked ? undefined : this.props.onInsertBlocksAfter }
onReplace={ isLocked ? undefined : onReplace }
mergeBlocks={ isLocked ? undefined : this.props.onMerge }
clientId={ clientId }
Expand Down Expand Up @@ -521,7 +513,6 @@ export class BlockListBlock extends Component {
/>
) }
<BlockDropZone
index={ order }
clientId={ clientId }
rootClientId={ rootClientId }
/>
Expand Down Expand Up @@ -612,6 +603,8 @@ export class BlockListBlock extends Component {
<Inserter
position="top right"
onToggle={ this.selectOnOpen }
rootClientId={ rootClientId }
clientId={ clientId }
/>
</div>
</Fragment>
Expand All @@ -634,7 +627,6 @@ const applyWithSelect = withSelect(
isFirstMultiSelectedBlock,
isTyping,
isCaretWithinFormattedText,
getBlockIndex,
getBlockMode,
isSelectionEnabled,
getSelectedBlocksInitialCaretPosition,
Expand Down Expand Up @@ -663,10 +655,9 @@ const applyWithSelect = withSelect(
isTypingWithinBlock:
( isSelected || isParentOfSelectedBlock ) && isTyping(),
isCaretWithinFormattedText: isCaretWithinFormattedText(),
order: getBlockIndex( clientId, rootClientId ),
mode: getBlockMode( clientId ),
isSelectionEnabled: isSelectionEnabled(),
initialPosition: getSelectedBlocksInitialCaretPosition(),
initialPosition: isSelected ? getSelectedBlocksInitialCaretPosition() : null,
isEmptyDefaultBlock:
name && isUnmodifiedDefaultBlock( { name, attributes } ),
isMovable: 'all' !== templateLock,
Expand Down Expand Up @@ -715,8 +706,20 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, { select } ) => {
insertBlocks( blocks, index, rootClientId );
},
onInsertDefaultBlockAfter() {
const { order, rootClientId } = ownProps;
insertDefaultBlock( {}, rootClientId, order + 1 );
const { clientId, rootClientId } = ownProps;
const {
getBlockIndex,
} = select( 'core/editor' );
const index = getBlockIndex( clientId, rootClientId );
insertDefaultBlock( {}, rootClientId, index + 1 );
},
onInsertBlocksAfter( blocks ) {
const { clientId, rootClientId } = ownProps;
const {
getBlockIndex,
} = select( 'core/editor' );
const index = getBlockIndex( clientId, rootClientId );
insertBlocks( blocks, index + 1, rootClientId );
},
onRemove( clientId ) {
removeBlock( clientId );
Expand Down Expand Up @@ -764,6 +767,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, { select } ) => {
} );

export default compose(
pure,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is useful to avoid rerendering all the blocks when we add/remove a block in the block list (parent component).

I was also tempted to replace it with React.memo as it may be more optimized than our HoC. It would be good to compare performance of these two.

withViewportMatch( { isLargeViewport: 'medium' } ),
applyWithSelect,
applyWithDispatch,
Expand Down
1 change: 0 additions & 1 deletion packages/editor/src/components/block-list/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,6 @@ class BlockList extends Component {
>
<BlockListBlock
clientId={ clientId }
index={ blockIndex }
blockRef={ this.setBlockRef }
onSelectionStart={ this.onSelectionStart }
rootClientId={ rootClientId }
Expand Down
9 changes: 4 additions & 5 deletions packages/editor/src/components/block-list/insertion-point.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class BlockInsertionPoint extends Component {
const {
showInsertionPoint,
rootClientId,
insertIndex,
clientId,
} = this.props;

return (
Expand All @@ -73,7 +73,7 @@ class BlockInsertionPoint extends Component {
>
<Inserter
rootClientId={ rootClientId }
index={ insertIndex }
clientId={ clientId }
/>
</div>
</div>
Expand All @@ -87,13 +87,12 @@ export default withSelect( ( select, { clientId, rootClientId } ) => {
isBlockInsertionPointVisible,
} = select( 'core/editor' );
const blockIndex = getBlockIndex( clientId, rootClientId );
const insertIndex = blockIndex;
const insertionPoint = getBlockInsertionPoint();
const showInsertionPoint = (
isBlockInsertionPointVisible() &&
insertionPoint.index === insertIndex &&
insertionPoint.index === blockIndex &&
insertionPoint.rootClientId === rootClientId
);

return { showInsertionPoint, insertIndex };
return { showInsertionPoint };
} )( BlockInsertionPoint );
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export function DefaultBlockAppender( {
value={ showPrompt ? value : '' }
/>
{ hovered && <InserterWithShortcuts rootClientId={ rootClientId } /> }
<Inserter position="top right" />
<Inserter rootClientId={ rootClientId } position="top right" isAppender />
</div>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ exports[`DefaultBlockAppender should append a default block when input focused 1
value="Start writing or type / to choose a block"
/>
<WithSelect(IfCondition(Inserter))
isAppender={true}
position="top right"
/>
</div>
Expand All @@ -48,6 +49,7 @@ exports[`DefaultBlockAppender should match snapshot 1`] = `
value="Start writing or type / to choose a block"
/>
<WithSelect(IfCondition(Inserter))
isAppender={true}
position="top right"
/>
</div>
Expand All @@ -71,6 +73,7 @@ exports[`DefaultBlockAppender should optionally show without prompt 1`] = `
value=""
/>
<WithSelect(IfCondition(Inserter))
isAppender={true}
position="top right"
/>
</div>
Expand Down
19 changes: 4 additions & 15 deletions packages/editor/src/components/inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,14 @@ class Inserter extends Component {
* @return {WPElement} Dropdown content element.
*/
renderContent( { onClose } ) {
const { rootClientId, index } = this.props;
const { rootClientId, clientId, isAppender } = this.props;

return (
<InserterMenu
onSelect={ onClose }
rootClientId={ rootClientId }
index={ index }
clientId={ clientId }
isAppender={ isAppender }
/>
);
}
Expand All @@ -100,27 +101,15 @@ class Inserter extends Component {
}

export default compose( [
withSelect( ( select, { rootClientId, index } ) => {
withSelect( ( select, { rootClientId } ) => {
const {
getEditedPostAttribute,
getBlockInsertionPoint,
hasInserterItems,
} = select( 'core/editor' );

if ( rootClientId === undefined && index === undefined ) {
// Unless explicitly provided, the default insertion point provided
// by the store occurs immediately following the selected block.
// Otherwise, the default behavior for an undefined index is to
// append block to the end of the rootClientId context.
const insertionPoint = getBlockInsertionPoint();
( { rootClientId, index } = insertionPoint );
}

return {
title: getEditedPostAttribute( 'title' ),
hasItems: hasInserterItems( rootClientId ),
rootClientId,
index,
};
} ),
ifCondition( ( { hasItems } ) => hasItems ),
Expand Down
Loading