Avoid unnecessary re-renders when navigating between blocks #11495

gziolo · 2018-11-05T15:25:42Z

Description

Another try to fix #11397 :)

As pointed out by @aduth isSelectionEnabled is almost always true so this check shouldn't work. It might be a bug which was apparently fixed in the meantime or me being wrong when testing. Anyways, this PR takes a bit different approach.

First of all, I moved onShiftSelection one level down as it doesn't depend on BlockList at all. This should make easier reasoning about its behavior next to the code which actually uses it.

Note to myself: ~~check if we can make it work as it would be a static method.~~ - dismissed.

The fix is documented inline. To avoid all re-renders we don't compute the value inside withSelect as it changes for all BlockListBlock instances. Instead we pass down selector and call it dynamically only when it's necessary. It is very rare use case, as it only happens when user holds down shift key and activates multi selection.

How has this been tested?

Screenshots

Before

After

Types of changes

Refactoring, no changes to the logic.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

…cks with keyboard

gziolo · 2018-11-05T15:53:48Z

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

+			return;
+		}
+
+		const { getBlockSelectionStart, onMultiSelect, onSelect } = this.props;


Actually, clientId can be destructured, too.

aduth · 2018-11-06T20:55:09Z

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

+
+		const { getBlockSelectionStart, onMultiSelect, onSelect } = this.props;
+
+		if ( getBlockSelectionStart() ) {


I really like the main refactoring of moving this handling to the individual block item, but I'm not sure how I feel about this specific change of calling a selector within the component itself. I don't think there's anything bad about it in the sense of generating inaccurate data, but it does feel like something which would be easy to overlook as far as the general pattern of providing data to components via a selector vs. passing the selector itself. That said, passing the selector does provide opportunities to optimize when the data is only needed in very specific circumstances like this.

Alternatively, I'm left to wonder if it's still something we could handle in the withDispatch callback, receiving the selection start from withSelect and somehow blocking it from reaching the component. I don't know that this is easily achievable without something like mapProps (to drop the unwanted prop) and a separate layer of pure. Not only is this pretty cumbersome, it probably has worse performance than if we were to just pass the data anyways.

A variation to this would be to still have the bulk of the handling occur in withDispatch, but call select to retrieve the value from getBlockSelectionStart. It's not too much different than what we're doing here, so I don't know that it really is much better.

What do you think?

You had even described it as confusing when @iseulde had proposed the same thing in #10431 😆

#10431 (comment)

You had even described it as confusing when @iseulde had proposed the same thing in #10431

Yes, I remember that. I still think that it is an issue. That's why I added an inline comment :)

A variation to this would be to still have the bulk of the handling occur in withDispatch, but call select to retrieve the value from getBlockSelectionStart. It's not too much different than what we're doing here, so I don't know that it really is much better.

I think the easiest change would be to add select as 3rd param to withDispatch HOC, which should work out of the box with the current implementation and would allow retrieving the value which causes this discussion inside the event handler without exposing it as a prop. Now, the question is if we want to confuse plugin developers with this new capability. I really like how symmetric and straightforward are withSelect and withDispatch as of today.

Alternatively, I'm left to wonder if it's still something we could handle in the withDispatch callback, receiving the selection start from withSelect and somehow blocking it from reaching the component. I don't know that this is easily achievable without something like mapProps (to drop the unwanted prop) and a separate layer of pure. Not only is this pretty cumbersome, it probably has worse performance than if we were to just pass the data anyways.

Redux solves this issue by offering 3rd param called mergeProps inside connect HOC. However, it's very difficult to use and even more difficult to read. Besides connect is one HOC so you can avoid re-renders by filtering unwanted props, but the cost is that you have to perform lots of computations to upfront to make it work.

It would be great if we would come up with a pattern to ensure we have a common way of handling similar use cases. In general, I observe that there are many places where we compute props which are never used for rendering but they sit there and wait until a given event occurs.

We can just pass select as a third argument in withDispatch for these advanced use-cases.

Edit I just saw that it was already proposed :)

I don't really like the idea, partly because with how proxying works, we shouldn't imply that withDispatch can be a method for injecting up-to-date state values to a component. In the very worst case, someone could even wrongly think to ignore withSelect altogether and just use withDispatch for all their needs.

I'm thinking more that wp.data.select within the withDispatch callback is maybe sensible, as its intended usage is for exactly these moments of one-off data retrieval.

Is the rerendering here a big issue to sacrifice devx like we're trying to do by finding solutions here? Maybe the current approach by @gziolo in the PR is just fine as it's well commented as a performance optimization. I'm thinking the generalization might not be worth it because it doesn't seem so common.

I don't think there is an easy solution, everything we do have some drawbacks. If you add 3rd params select to withDispatch it might be used incorrectly. Example: when you call selector outside of the event handler, it might not re-evaluate properly when the value you want to defer changes because it isn't reflected in props. So you still need to make sure it's called inside the handler.

Is the rerendering here a big issue to sacrifice devx like we're trying to do by finding solutions here?

Yes, I think so. More-so being that this is BlockListBlock.

I see it as something we should consider having some guidelines around, since it's not the first time this has come up.

What's the drawback with calling the main select (the one exported by @wordpress/data) from the withDispatch callback?

Yes, I think so. More-so being that this is BlockListBlock.

Agreed with this, that we should make this particular component performant (and the current approach is also sacrificing good code, because passing selector is not great either but it's localized)

What I'm not certain about is the changes we need to do in the data module to accommodate this.

gziolo · 2018-11-09T09:27:14Z

Should we proceed with the current approach and find a better solution when we have more use cases which would justify the need of introducing another param in withDispatch or building a new HOC?

aduth · 2018-11-09T14:10:22Z

Since I'm thinking my suggestion may have been unclear, this is what I had in mind:

https://github.com/WordPress/gutenberg/compare/master...try/list-render-select-with-dispathc?expand=1

youknowriad · 2018-11-09T14:12:36Z

@aduth I thought I replied to this but apparently not :) I want us to avoid the singleton as much as we can going forward. The drawback is that you can be using a separate registry (think reusable block for instance)

aduth · 2018-11-09T14:14:13Z

Oh, I see what you mean, since it wouldn't respect the registry via context in which the component is being rendered.

aduth

I'm fine with this for now. I'm not sure how we can better make this more clear, but I think it's reasonable to have exceptional handling of selectors for this component in particular.

aduth · 2018-11-09T14:15:09Z

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

@@ -645,6 +665,9 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps ) => {
 		onSelect( clientId = ownProps.clientId, initialPosition ) {
 			selectBlock( clientId, initialPosition );
 		},
+		onMultiSelect( ...args ) {


This could just be onMultiSelect: multiSelect

Performance: Avoid unnecessary re-renders when navigating between blo…

4183bbf

…cks with keyboard

gziolo requested a review from aduth November 5, 2018 15:25

gziolo self-assigned this Nov 5, 2018

gziolo added [Type] Performance Related to performance efforts [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Enhancement A suggestion for improvement. labels Nov 5, 2018

gziolo added this to the 4.3 milestone Nov 5, 2018

gziolo commented Nov 5, 2018

View reviewed changes

aduth reviewed Nov 6, 2018

View reviewed changes

gziolo requested a review from a team November 7, 2018 14:44

mtias changed the title ~~Performance: Avoid unnecessary re-renders when navigating between blo…~~ Avoid unnecessary re-renders when navigating between blocks Nov 8, 2018

aduth approved these changes Nov 9, 2018

View reviewed changes

Small tweak per review

bf454c2

youknowriad merged commit 660e46e into master Nov 9, 2018

youknowriad deleted the fix/re-renders-block-list-shift branch November 9, 2018 17:21

aduth mentioned this pull request Nov 14, 2018

Add registry param to withDispatch component #11851

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unnecessary re-renders when navigating between blocks #11495

Avoid unnecessary re-renders when navigating between blocks #11495

gziolo commented Nov 5, 2018 •

edited by mtias

gziolo Nov 5, 2018

aduth Nov 6, 2018

aduth Nov 6, 2018

gziolo Nov 7, 2018

youknowriad Nov 7, 2018 •

edited

aduth Nov 8, 2018

aduth Nov 8, 2018 •

edited

youknowriad Nov 9, 2018

gziolo Nov 9, 2018 •

edited

aduth Nov 9, 2018 •

edited

youknowriad Nov 9, 2018

gziolo commented Nov 9, 2018

aduth commented Nov 9, 2018

youknowriad commented Nov 9, 2018 •

edited

aduth commented Nov 9, 2018

aduth left a comment

aduth Nov 9, 2018


		const { getBlockSelectionStart, onMultiSelect, onSelect } = this.props;

		if ( getBlockSelectionStart() ) {

Avoid unnecessary re-renders when navigating between blocks #11495

Avoid unnecessary re-renders when navigating between blocks #11495

Conversation

gziolo commented Nov 5, 2018 • edited by mtias

Description

How has this been tested?

Screenshots

Before

After

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Nov 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Nov 9, 2018 • edited

Choose a reason for hiding this comment

aduth Nov 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 9, 2018

aduth commented Nov 9, 2018

youknowriad commented Nov 9, 2018 • edited

aduth commented Nov 9, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 5, 2018 •

edited by mtias

youknowriad Nov 7, 2018 •

edited

aduth Nov 8, 2018 •

edited

gziolo Nov 9, 2018 •

edited

aduth Nov 9, 2018 •

edited

youknowriad commented Nov 9, 2018 •

edited