Avoid getBlock in block-list/block by youknowriad · Pull Request #11899 · WordPress/gutenberg

youknowriad · 2018-11-15T09:55:02Z

Last part to be extracted from #11811. This PR tries to make The BlockListBlock component more performant by avoiding the use of getBlock (which is not great especially for columns blocks)

aduth · 2018-11-26T20:37:42Z

Do you intend / have a desire to see this through before 5.0 ? I have a sense it could be quite impactful, particularly for nested blocks, which see quite a bit of unnecessary re-renders on child updates (I'd hope would not occur with the refactor?).

Might go well with #12312

youknowriad · 2018-11-26T20:39:23Z

My hope is to get this as soon as possible. I'd like to measure the impact first. And yeah I think with this #12312 it seems logical to have a separate selector for the attributes.

I think it's probably too late for 5.0 though but maybe good for 5.0.1

youknowriad · 2018-11-26T20:40:00Z

I'll try to rebase and measure this tomorrow.

youknowriad · 2018-11-27T08:22:07Z

In my testing this represents a 13% performance increase over master (for keypress events). But I'd appreciate confirmation from others @sgomes @aduth

aduth · 2018-11-27T13:54:57Z

The test failure looks legitimate:

[1]   ● align › withDataAlign › should not render wide/full wrapper props if wide controls are not enabled
[1] 
[1]     TypeError: Cannot read property 'align' of undefined
[1] 
[1]       143 | 	( props ) => {
[1]       144 | 		const { blockName, blockAttributes, hasWideEnabled } = props;
[1]     > 145 | 		const { align } = blockAttributes;
[1]           | 		        ^
[1]       146 | 		const validAlignments = getValidAlignments(
[1]       147 | 			getBlockSupport( blockName, 'align' ),
[1]       148 | 			hasBlockSupport( blockName, 'alignWide', true ),
[1] 
[1]       at align (packages/editor/src/hooks/align.js:145:11)
[1]       at mountIndeterminateComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6889:13)
[1]       at beginWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:7389:16)
[1]       at performUnitOfWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10149:12)
[1]       at workLoop (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10181:24)
[1]       at renderRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10267:7)
[1]       at performWorkOnRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11135:7)
[1]       at performWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11047:7)
[1]       at performSyncWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11021:3)
[1]       at requestWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10890:5)
[1]       at scheduleWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10700:5)
[1]       at scheduleRootUpdate (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11287:3)
[1]       at updateContainerAtExpirationTime (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11315:10)
[1]       at updateContainer (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11326:10)
[1]       at Object.create (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11842:5)
[1]       at Object.create (packages/editor/src/hooks/test/align.js:202:29)

aduth · 2018-11-27T21:11:35Z

packages/editor/src/components/block-list/block-invalid-warning.js

-	const { replaceBlock } = dispatch( 'core/editor' );
+export default compose( [
+	withSelect( ( select, { clientId } ) => ( {
+		block: select( 'core/editor' ).getBlock( clientId ),


This looks like a good candidate for optimization when #11851 lands.

Edit: I guess it probably won't have much an impact in common usage, if the block warning is not expected to be frequently shown. I don't see any downside to standardizing on a convention of using the registry argument if the selected prop is only used in the callback of a withDispatch function though.

aduth · 2018-11-27T21:15:40Z

packages/editor/src/components/block-list/block.js

 						isDraggable,
 						className,
+						blockName,
+						isValid,


It seems odd our mixed application of "block" as a prefix in props, when in fact the isValid is specifically referring to the validity of a block. Given that the component is "BlockListBlock", an argument could be made that the prefix is not necessary at all.

Not really a blocker.

Not really a blocker.

I love it :) I think I'll update and remove the prefix (I hope I won't have eslint issues :) )

aduth · 2018-11-27T21:18:30Z

packages/editor/src/components/block-list/block.js

-		isEmptyDefaultBlock: block && isUnmodifiedDefaultBlock( block ),
-		isPreviousBlockADefaultEmptyBlock: previousBlock && isUnmodifiedDefaultBlock( previousBlock ),
+		isEmptyDefaultBlock: blockName && isUnmodifiedDefaultBlock( { name: blockName, attributes: blockAttributes } ),
+		isPreviousBlockADefaultEmptyBlock: previousBlockClientId && isUnmodifiedDefaultBlock( {


My concern from #11811 (comment) still holds true here.

Not strictly related to the pull request, but: Why do we care about not showing a sibling inserter if the previous block is an empty default block? I might do some separate exploring 😄

This was @jasmussen's proposal I think. I also would prefer removing it if it doesn't harm UX

Just to be sure I understand this correctly: this rule hides the sibling inserter between an empty paragraph block and another block?

If correct, the reasoning for removing that likely stems back to when the sibling inserter click area spanned the full width and it was way too easy to invoke it. Given recent changes to this hit area and the sibling inserter behavior itself, totally okay to revisit and see if it's a good experience.

aduth · 2018-11-28T20:56:36Z

docs/designers-developers/developers/filters/block-filters.md

-		wrapperProps = { ...wrapperProps, 'data-align': align };
-
-		return <BlockListBlock { ...props } wrapperProps={ wrapperProps } />;
+		return <BlockListBlock { ...props } className={ "block-" + props.clientID } />;


clientID -> clientId

aduth

With accumulating approved pull requests, we're prone to start having some troublesome merge conflicts. Looking at #12312 and #12379 in particular.

aduth · 2018-11-28T21:04:27Z

packages/editor/src/store/selectors.js

 		return {
 			...block,
-			attributes,
+			attributes: getBlockAttributes( state, clientId ),


We should maybe consider revising the dependants of this selector to use getBlockAttributes.getDependants( state, clientId ) instead of listing them out again explicitly.

* Avoid rerending the current block if the previous block change * Always show the sibling inserter * Optimize the insertion point component (#12384)

phpbits · 2018-12-01T10:25:37Z

@youknowriad @aduth I'm having issue, probably because of this update. Here's the info : #12494 . Thanks!

aduth · 2018-12-12T14:54:21Z

packages/editor/src/components/block-list/block.js

+			isSelectionEnabled: isSelectionEnabled(),
+			initialPosition: getSelectedBlocksInitialCaretPosition(),
+			isEmptyDefaultBlock:
+				name && isUnmodifiedDefaultBlock({ name, attributes }),


This auto-formatting sure did make a mess 😕 (even after "lint fixes")

ktmn · 2018-12-17T06:57:29Z

Is this a breaking change for the editor.BlockListBlock hook? At least that's what I'm experiencing with WP 5.0.2-RC1-44259, since one of my functions expects props.block in that action.

So now I do select('core/editor').getBlock(clientId), which gets me the block, but does that basically negate any performance wins from this PR? I need the block name and/or it's attributes, is there a cheaper way to get these 2? Because currently it's doing select('core/editor').getBlock() for every single block in use in the editor just to check for hasBlockSupport().

If there are 2 3rd party actions hooked to editor.BlockListBlock that need the block is it now less performant as select('core/editor').getBlock() would be called by both of them?

aduth · 2018-12-18T15:27:55Z

@ktmn These changes are largely reverted as of #12943 (available in 5.0.2-RC2). Exposing all of the props was deemed here to be a non-intentional exposure, where the clientId was the primary interface target†. Generally I'd say it would be better for you to use the clientId prop in combination with getBlockName and getBlockAttributes selectors, though as it stands the block prop is once again passed through to filters, and for compatibility's sake I'm not sure this will change.

† The reason for this being that BlockListBlock is a pivotal component in the editor rendering, and is fully expected to be refactored as necessary for performance or features' sake, including adding or removing props.

youknowriad added the [Type] Performance Related to performance efforts label Nov 15, 2018

youknowriad self-assigned this Nov 15, 2018

youknowriad requested review from a team and aduth November 15, 2018 09:55

youknowriad force-pushed the update/avoid-get-block-block-component branch from 972b7a4 to 0d69dc8 Compare November 27, 2018 08:02

mtias added this to the 4.7 milestone Nov 27, 2018

aduth reviewed Nov 27, 2018

View reviewed changes

mtias added the [Feature] Blocks Overall functionality of blocks label Nov 28, 2018

youknowriad mentioned this pull request Nov 28, 2018

Avoid rerending the current block if the previous block change #12379

Merged

aduth reviewed Nov 28, 2018

View reviewed changes

aduth approved these changes Nov 28, 2018

View reviewed changes

youknowriad force-pushed the update/avoid-get-block-block-component branch from 78c74f1 to 26c9e96 Compare November 30, 2018 13:02

youknowriad added 9 commits November 30, 2018 14:55

Avoid getBlock in block-list/block

af47557

Update data docs

ffcd3c7

Fix unit tests post rebase :)

bcb0c0a

Rename props

9b9bfc4

Clarify the filter position

f273996

Avoid repeating the selector dependants

94658f0

Fix small typo in the docs

12f91a5

Avoid rerending the current block if the previous block change (#12379)

69aa303

* Avoid rerending the current block if the previous block change * Always show the sibling inserter * Optimize the insertion point component (#12384)

Fix lock file

81c5791

youknowriad force-pushed the update/avoid-get-block-block-component branch from ca7aac5 to 81c5791 Compare November 30, 2018 13:56

Fix linting

cda9eef

youknowriad force-pushed the update/avoid-get-block-block-component branch from 23d5c70 to cda9eef Compare November 30, 2018 14:25

youknowriad merged commit 51e45ba into master Nov 30, 2018

youknowriad deleted the update/avoid-get-block-block-component branch November 30, 2018 14:57

youknowriad mentioned this pull request Dec 1, 2018

editor.BlockListBlock not updating on attribute change. #12494

Closed

This was referenced Dec 3, 2018

Improve typing performance by splitting attributes in state tree. #12312

Merged

Performance: Improve the initial loading time and the feeling fo smoothness while typing in blocks. #11782

Closed

aduth reviewed Dec 12, 2018

View reviewed changes

aduth mentioned this pull request Dec 12, 2018

Editor: Handle block merge in withDispatch callback #12820

Merged

pento mentioned this pull request Dec 17, 2018

Removing getBlock from the BlockListBlock component breaks back compat #12938

Closed

aduth mentioned this pull request Dec 17, 2018

Restore block prop for BlockListBlock filter #12943

Merged

Conversation

youknowriad commented Nov 15, 2018

Uh oh!

aduth commented Nov 26, 2018

Uh oh!

youknowriad commented Nov 26, 2018

Uh oh!

youknowriad commented Nov 26, 2018

Uh oh!

youknowriad commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phpbits commented Dec 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktmn commented Dec 17, 2018

Uh oh!

aduth commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

youknowriad commented Nov 27, 2018 •

edited

Loading

aduth commented Dec 18, 2018 •

edited

Loading