Avoid rerending the current block if the previous block change by youknowriad · Pull Request #12379 · WordPress/gutenberg

youknowriad · 2018-11-28T10:23:35Z

This PR builds on top of #11899 And it's another refactoring to the BlockListBlock component to avoid useless selectors.

In my testing. Key press events are 22% faster in long posts.

youknowriad · 2018-11-28T10:24:11Z

packages/blocks/src/api/utils.js

Since we're using strings now for RichText values, the isEqual is useless I think.

=== will check that the values are of the same type and that their values match, so it'll work nicely in the context of two strings, yes.

Since we're using strings now for RichText values, the isEqual is useless I think.

I also don't think it's something we should have ever wanted, even for non-primitive types, since even if the values are deeply equal, the test seems like it should be more concerned about whether the value has ever changed from its default-assigned reference.

the value has ever changed from its default-assigned reference.

The reason was: you type in a paragraph, you backspace, you click outside of the block. You have an empty paragraph but a new reference.

And to me I'd be fine to call that "modified" 🤷‍♂️

youknowriad · 2018-11-28T10:25:19Z

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

As discussed here #11899 (comment) this means we now show the inserter even if the previous block is empty.

Nice removal! Should be an excellent improvement not to load two blocks every single time.

youknowriad · 2018-11-28T10:25:57Z

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

Internal filter not useful anymore because we got rid of "layout" attributes.

sgomes

The code changes look good to me. Great work, @youknowriad! This is an excellent example of where a small UX compromise can lead to a big performance boost.

I'll try to replicate your results in the meantime, but code-wise looks good to merge 👍

sgomes · 2018-11-28T11:32:26Z

I see a similar improvement (23%) in my machine 👍

aduth

I like this in an effort of reducing the unpredictability around when and where a sibling inserter is shown. One thing I might worry about is: Before we'd never shown a sibling inserter around an empty default block at all, and now we show it after but not before an empty default block. It seems to me we'd probably be better off one way or the other. In other words, do we even need this canShowInBetweenInserter at all ?

aduth · 2018-11-28T13:47:01Z

packages/blocks/src/api/utils.js

Since we're using strings now for RichText values, the isEqual is useless I think.

I also don't think it's something we should have ever wanted, even for non-primitive types, since even if the values are deeply equal, the test seems like it should be more concerned about whether the value has ever changed from its default-assigned reference.

aduth · 2018-11-28T13:50:14Z

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

-		const { clientId, previousBlockClientId, nextBlockClientId, onMerge } = this.props;
-
+		const { clientId, getPreviousBlockClientId, getNextBlockClientId, onMerge } = this.props;
+		const previousBlockClientId = getPreviousBlockClientId( clientId );


Another thing which will be nice to move to the dispatch handler when #11851 lands. Or, at least, our non-conventional use of selectors as functions directly within the component is concerning (but largely in its being unconventional).

Or, at least, our non-conventional use of selectors as functions directly within the component is concerning (but largely in its being unconventional).

On further reflection, I think I could articulate my concern better in being a diminished distinction between withSelect as serving to provide data and withDispatch as serving to provide behavior, where what we've done here is more in the realm of a behavior.

To clarify, not something which needs to be addressed here.

aduth · 2018-11-28T13:51:02Z

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

 		isSelected,
 		isParentOfSelectedBlock,
+
 		// We only care about this value when the shift key is pressed.


This comment is no longer strictly applicable to the use-case, since there's now a second use-case.

aduth · 2018-11-28T13:54:17Z

packages/blocks/src/api/utils.js

-
-	return every( attributeKeys, ( key ) =>
-		isEqual( newDefaultBlock.attributes[ key ], block.attributes[ key ] )
+	return every( blockType.attributes, ( value, key ) =>


There's a subtlety here in how we use the block type's attributes definition instead of the actual default block we've created; I assume that's so we account for keys which don't have a default assigned and thus would be absent from the default block, but potentially present in the block being tested?

Some comment to this effect could help clarify for future maintainers.

My thinking is that the previous implementation was needed because undeclared attributes were persisted initially, but that's not the case anymore.

aduth · 2018-11-28T13:57:02Z

packages/blocks/src/api/utils.js

@@ -40,14 +39,10 @@ export function isUnmodifiedDefaultBlock( block ) {
 	}

 	const newDefaultBlock = createBlock( defaultBlockName );


Maybe a red herring, but createBlock seems like something which in its current state is somewhat non-trivial, at least if we expect it to be called frequently. Do we need to create a new instance of the block each time this function is called?

We need the "normalized" attributes (default values), An alternative would be to avoid this behavior "isUnmodified" if the default block has been changed. And just rely on very quick but static checks as we know the block is "paragraph".

youknowriad · 2018-11-28T14:16:35Z

In other words, do we even need this canShowInBetweenInserter at all ?

I don't know, I'll explore it separately :)

youknowriad · 2018-11-28T14:16:55Z

or actually, I'll update this PR iff it seems useless.

aduth · 2018-11-28T14:27:52Z

In other words, do we even need this canShowInBetweenInserter at all ?

I don't know, I'll explore it separately :)

or actually, I'll update this PR iff it seems useless.

My worry is that the pull request in its current form is the worst of all options, between:

Sibling inserter shown neither before nor after an empty block (master)
Sibling inserter shown after, but not before an empty block (this branch in its current incarnation)
Sibling inserter shown before and after an empty block

youknowriad · 2018-11-28T14:34:03Z

I pushed a commit to always show the in-between inserter.

aduth · 2018-11-28T15:43:44Z

I pushed a commit to always show the in-between inserter.

FYI @jasmussen

aduth · 2018-11-28T15:49:13Z

The insertion point line is not shown when hovering a block option in the inserter before an empty block. Am I correct in assuming that this would be resolved by #12384 ?

youknowriad · 2018-11-28T15:59:09Z

I think that's correct @aduth

now that you mention that, I think I know why we don't show the inserter and the insertion point for empty blocks, that's because we replace them instead of adding new blocks when you choose a block from the inserter.

I personally think it's a fine "compromise" for performance to show them even if we replace the block instead of inserting new ones.

aduth · 2018-11-28T16:01:37Z

that's because we replace them instead of adding new blocks when you choose a block from the inserter.

I find that a bit peculiar as well 😆 But it's neither here nor there, as I don't really find this to really have an effect (negative or positive) on the expectation of that behavior.

mtias · 2018-11-30T09:52:23Z

Nice to see this improvement!

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

* Avoid getBlock in block-list/block * Update data docs * Fix unit tests post rebase :) * Rename props * Clarify the filter position * Avoid repeating the selector dependants * Fix small typo in the docs * Avoid rerending the current block if the previous block change (#12379) * Avoid rerending the current block if the previous block change * Always show the sibling inserter * Optimize the insertion point component (#12384) * Fix lock file * Fix linting

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

youknowriad self-assigned this Nov 28, 2018

youknowriad requested review from a team, aduth and sgomes November 28, 2018 10:23

youknowriad commented Nov 28, 2018

View reviewed changes

mtias added this to the 4.7 milestone Nov 28, 2018

youknowriad commented Nov 28, 2018

View reviewed changes

sgomes approved these changes Nov 28, 2018

View reviewed changes

Avoid rerending the current block if the previous block change

c3987ff

youknowriad force-pushed the perf/avoid-previous-block-selector branch from 849c449 to c3987ff Compare November 28, 2018 12:41

aduth approved these changes Nov 28, 2018

View reviewed changes

Always show the sibling inserter

9bb0a3c

mtias added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Nov 28, 2018

Optimize the insertion point component (#12384)

5216124

youknowriad merged this pull request into update/avoid-get-block-block-component Nov 30, 2018

youknowriad deleted the perf/avoid-previous-block-selector branch November 30, 2018 08:15

aduth mentioned this pull request Dec 4, 2018

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

Closed

		@@ -40,14 +39,10 @@ export function isUnmodifiedDefaultBlock( block ) {
		}

		const newDefaultBlock = createBlock( defaultBlockName );

Conversation

youknowriad commented Nov 28, 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!

sgomes left a comment

Choose a reason for hiding this comment

Uh oh!

sgomes commented Nov 28, 2018

Uh oh!

aduth left a comment

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!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Nov 28, 2018

Uh oh!

youknowriad commented Nov 28, 2018

Uh oh!

aduth commented Nov 28, 2018

Uh oh!

youknowriad commented Nov 28, 2018

Uh oh!

aduth commented Nov 28, 2018

Uh oh!

aduth commented Nov 28, 2018

Uh oh!

youknowriad commented Nov 28, 2018

Uh oh!

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

Uh oh!

Uh oh!

mtias commented Nov 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aduth commented Nov 28, 2018 •

edited

Loading