Data: Provide dependencies from withSelect to useSelect

[aduth]

Previously: #15737

This pull request proposes to include a dependencies array in withSelect's rendering of useSelect. The goal here is to reduce the number of calls to mapSelectToProps, since otherwise the mapping callback would be called in every render. From my reading of #15737, I see that there is pretty extensive discussion of how dependencies should be managed, so perhaps I am overlooking some context here. I see that pure is used to avoid unnecessary renders of the withSelect higher-order component. Since mapSelectToProps depends on ownProps, it must be defined as a dependency of the useSelect hook if dependencies are passed. Given pure should enforce that ownProps reference changes only upon an effective shallow change in props, it should be enough to pass this object directly as the sole dependency to useSelect. I think there may have been some expectation that pure would be enough to avoid excessive calls to mapSelect, but since the component could render for reasons other than just withSelect, useSelect's internal reference of useCallback would always change when provided an undefined deps array, and thus always call mapSelect even without a change in props or global state.

With these changes, I observe a decrease of mapSelect calls on an initial rendering of the editor from 157 to 112. This is intended to serve as a simple reference point; it's expected this benefit should sustain over lifetime of the editor session.

This follows some discussion at #18960 (comment) and for me is something of a personal study of if and how dependencies should apply to hooks, or if dependencies should always be provided when supported by a hook.

Performance Results:

In recent performance testing of other pull request, I've struggled to find any measurable difference (better or worse) using npm run test-performance. I'm not sure if this is an issue with my environment, the tests as written, or the fact that there's truly little benefit to these changes.

For exhaustiveness' sake, I dove into the React source to seek some insight into whether the overhead of comparing dependencies in renders might incur a higher cost than any benefit to be gained by avoiding a call to mapSelect. This operation appears to be very trivial; not much more than a simple loop of the before/after deps [1] [2] [3].

Testing Instructions:

It's assumed unit tests and end-to-end test cases should protect against regressions here.

[nerrad]

We tried this when originally developing the useSelect hook. The problem with using ownProps as a dependency is because at the time the shape/size of the ownProps could change between re-calculations causing an error with react.

I don't think pure or React.memo usage here changes that. In the end, that's why it's preferable to use useSelect directly where the dependencies array can be used effectively. For withSelect there's just too many unknowns regarding the shape/size of ownProps which we could probably address internally in Gutenberg, but not for all the projects implementing withSelect in the wild.

[aduth]

The problem with using ownProps as a dependency is because at the time the shape/size of the ownProps could change between re-calculations causing an error with react.

I'm not sure I understand this point. As implemented in this pull request (props passed as a member of the dependencies array), React isn't inspecting the object itself, only considering whether the reference value of the object differs from the previous reference value (i.e. ownProps !== prevOwnProps). pure is relevant in that it adds a guarantee that the the reference value of props should only change when they are in fact shallow-inequal.

I see in the discussion there that there was some back-and-forth about how to implement pure, but ultimately it was implemented. So the original approach to pass [ ownProps ] would be okay, then?

[nerrad]

This is the React warning I got at the time (looks like it's still in source):

Warning: The final argument passed to useCallback changed size between renders. The order and size of this array must remain constant.

So the problem is if on render calc one you have something like this for ownProps:

const ownProps = { foo: 'bar', cheeseburgers: 'fries' };

And then on future render calc this was instead:

const ownProps = { foo: 'bar' };

This is something that could typically happen if there is variation on the number of props passed through on subsequent render calcs in the tree. Of course all this is based on what I experienced at the time.

[nerrad]

Oh dang, sorry. I just realized, I was getting the error because at the time I was trying to expand the props into the dependency array and that's why I was getting the warning. Ignore everything I said.

[aduth]

Yes, I could see this being a problem if this were implemented as:

useSelect( mapSelect, ownProps );

Or some derivation from it:

useSelect( mapSelect, Object.values( ownProps ) );

But if it's passed as a member of an array (as in this pull request):

useSelect( mapSelect, [ ownProps ] );

... then React shouldn't be bothering to inspect it aside from === reference equality.

[youknowriad]

ownProps is a shallow object in general, should we instead try to extract the values from that object as an array (and try to keep the order based on prop names)?

[aduth]

ownProps is a shallow object in general, should we instead try to extract the values from that object as an array (and try to keep the order based on prop names)?

I don't think so, no. For the same reasons as discussed from earlier comments, your suggestion may be considered problematic for how React compares dependencies. But I also don't think it's necessary that we do this. pure on the withSelect should be enough to keep reference value updates limited to effective changes.

Even if we were to go down this route, I think it would greatly diminish any potential benefit we'd gain over simply running the selector on every render, since trying to derive the dependencies from ownProps would itself be a fairly expensive operation.

[nerrad]

ownProps is a shallow object in general, should we instead try to extract the values from that object as an array (and try to keep the order based on prop names)?

This is exactly what I had tried in the original useSelect pull which led to the React warning I mentioned (which as I noted later is not relevant to this particular pull as I originally thought).

[nerrad]

I don't think this pull should hurt anything as it's implementing useSelect using a pattern already used elsewhere. A side-benefit is it models a pattern that should be used for any newer developers diving through our code.

I'll approve this, but there's still a number of failing e2e tests. On the surface it doesn't look like they should be related to this pull (I didn't look closely at any) but I'm assuming they'll be examined prior to merge.

[epiqueras]

This is breaking tests because withSelect has always been expected to run mapSelect when props.isAsync changes.

In useSelect, changing async mode merely changes a flag so that subsequent updates are queued instead of being ran immediately.

The only reasons for withSelect to re-render are:

• Props changed.
• const registry = useRegistry(); changed.
• const isAsync = useAsyncMode(); changed.

For each of those reasons mapSelect might or might not be re-ran depending on if it's memoized.

✅ means it re-ran.
❌ means it didn't.

Changed Value	Memoized	Not Memoized
Props	✅	✅
Registry	✅	✅
isAsync	❌	✅

By not memoizing mapSelect in withSelect, we keep backwards compatibility.

[aduth]

withSelect has always been expected to run mapSelect when props.isAsync changes.

Where is this dependency established between withSelect and isAsync? Is it actually a prop to withSelect, or a dependency on that context value? Is this something which should be a concern of withSelect, or is it useSelect which should be including this as a dependency to useCallback in its own internal references to isAsync and registry (I suppose this is more a question of whether there's some backwards-compatibility concern that keeps us to this commitment)?

Also a bit unsure if you're proposing that this change be abandoned, or if it's something where we can more-explicitly define this dependency between withSelect and isAsync (i.e. include useAsyncMode() in withSelect, and provide it as an extra dependency for useSelect).

[epiqueras]

Here:

gutenberg/packages/data/src/components/with-select/index.js

Lines 133 to 150 in ba39b5d

	if ( hasPropsChanged || hasSyncRenderingChanged ) {
	const nextMergeProps = getNextMergeProps( nextProps );
	if ( ! isShallowEqualObjects( this.mergeProps, nextMergeProps ) ) {
	// If merge props change as a result of the incoming props,
	// they should be reflected as such in the upcoming render.
	// While side effects are discouraged in lifecycle methods,
	// this component is used heavily, and prior efforts to use
	// `getDerivedStateFromProps` had demonstrated miserable
	// performance.
	this.mergeProps = nextMergeProps;
	}
	// Regardless whether merge props are changing, fall through to
	// incur the render since the component will need to receive
	// the changed `ownProps`.
	}
	return true;

We need the behavior in withSelect to keep backwards compatibility, but if useSelect implements this internally then it would introduce the performance concern this PR addresses, everywhere useSelect is used as well.

I think we can avoid that without any repercussions, but making the dependency more explicit by providing the async mode as a dependency to useSelect is a great idea.

[aduth]

I think we can avoid that without any repercussions, but making the dependency more explicit by providing the async mode as a dependency to useSelect is a great idea.

Okay, I can do that.

But I'm still not entirely clear on why we need to retain the previous behavior. We don't otherwise treat a refactor from withSelect to useSelect as a potentially-breaking change based on this behavior, so why can't we also just remove this expectation around how isAsync impacts withSelect ? Aside from the unit tests, what meaningful user-impacting breakage would we expect?

[epiqueras]

We don't otherwise treat a refactor from withSelect to useSelect as a potentially-breaking change based on this behavior

Technically, in very-edge cases, it is.

so why can't we also just remove this expectation around how isAsync impacts withSelect ? Aside from the unit tests, what meaningful user-impacting breakage would we expect?

None I can think of, but we would break third party tests. If the impact of that is not too big, we can add a dev note and move on I guess.

[aduth]

We don't otherwise treat a refactor from withSelect to useSelect as a potentially-breaking change based on this behavior

Technically, in very-edge cases, it is.

I don't think this is clear to most people who are refactoring to use useSelect in place of withSelect (at least, it's not to me). So I think either we'd want to clearly communicate what these breaking changes are, or make the behavior uniform, even if that means breaking from how withSelect would have worked previously.

My preference would be to the latter but, as noted, I also don't feel confident in knowing what effective impact these changes have 😄

If it's agreeable, I can work to update the unit tests so they pass.

[epiqueras]

I don't think this is clear to most people who are refactoring to use useSelect in place of withSelect (at least, it's not to me).

Yeah, definitely not.

So I think either we'd want to clearly communicate what these breaking changes are, or make the behavior uniform, even if that means breaking from how withSelect would have worked previously.
My preference would be to the latter but, as noted, I also don't feel confident in knowing what effective impact these changes have 😄

I also favor the latter approach to make things consistent. The "breaking change" isn't really breaking anything, but very specific implementation tests. At least, that's what it looks like now.

If it's agreeable, I can work to update the unit tests so they pass.

Let's do it. If anything big breaks, it will be easy to revert.

[aduth]

Verifying the changes from updating the tests, the other test updates make sense to me, but this one doesn't. I'm not sure why OriginalComponent renders an extra time as a result of these changes. And it seems to happen any time a dependencies array is provided to useSelect, regardless of whether it's a fixed value or not (i.e. still renders an extra time when providing []). I'm not sure if this is a specific characteristic of useCallback with dependencies (possibly related to facebook/react#14099?) and whether it really only has impact under a changing registry context that this test case is verifying against. It wouldn't be quite as worrying if it is only for the changing registry, since that doesn't happen quite so often in real-world usage.

[epiqueras]

The first render is for the registry changing, and the second one is for the return value of mapSelectToProps changing, right?

It seems to check out. We could also add a test that changes the state without changing the registry and compare.

[epiqueras]

The first render is for the registry changing, and the second one is for the return value of mapSelectToProps changing, right?

It seems to check out. We could also add a test that changes the state without changing the registry and compare.

[aduth]

The first render is for the registry changing, and the second one is for the return value of mapSelectToProps changing, right?

Yes, I think that's what the original tests were trying to verify. But now there's a third unexplained render.

[epiqueras]

It was only called once before.

[epiqueras]

The mock count carries on from the previous render.

[aduth]

I'm referring to the change of:

Before:

expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );

After:

expect( OriginalComponent ).toHaveBeenCalledTimes( 3 );

[epiqueras]

Mmmm, have you tried seeing if ownProps is somehow changed/invalidated? That could trigger an extra render.

[aduth]

Based on the failing end-to-end tests, there's definitely an impact of these changes.

In this test, for example, the ArrowLeft second-to-last action will create a list item at the top-most list indentation, rather than the behavior on master where it moves the caret into the nested list item:

gutenberg/packages/e2e-tests/specs/editor/blocks/list.test.js

Lines 459 to 489 in 3687e8b

	it( 'should preserve indentation after merging backward and forward', async () => {
	await clickBlockAppender();
	// Tests the shortcut with a non breaking space.
	await page.keyboard.type( '* 1' );
	await page.keyboard.press( 'Enter' );
	await page.keyboard.press( 'Space' );
	await page.keyboard.type( '2' );
	await page.keyboard.press( 'Enter' );
	await page.keyboard.type( '3' );
	// Create a new paragraph.
	await page.keyboard.press( 'Enter' );
	await page.keyboard.press( 'Enter' );
	// Merge the pragraph back. No list items should be joined.
	await page.keyboard.press( 'Backspace' );
	expect( await getEditedPostContent() ).toMatchSnapshot();
	// Again create a new paragraph.
	await page.keyboard.press( 'Enter' );
	// Move to the end of the list.
	await page.keyboard.press( 'ArrowLeft' );
	// Merge forward. No list items should be joined.
	await page.keyboard.press( 'Delete' );
	expect( await getEditedPostContent() ).toMatchSnapshot();
	} );

It's not really clear to me why this happens. It could still beg the question whether this is a change we want to make, but I suspect if the direction is to migrate to useSelect for these components, we'd eventually need to deal with it anyways (cc @ellatrix).

[epiqueras]

Sounds like timing issues with async mode and stale props.

Maybe RichText was relying on async mode changing to update its props, before an actual subscription or ownProps change?

[aduth]

Was hopeful that #19286 and #19282 would be enough to clear up the failing end-to-end tests, but there still appears to be a few legitimate (at least consistent) ones remaining.

[epiqueras]

Can you reproduce them outside of tests? Maybe it's just the tests not waiting long enough for something that before updated earlier due to unnecessary reruns?

[aduth]

There's less value in these changes over time as we continue to migrate more components to use useSelect as the preferred data consumer. Given the challenges which have been encountered thus far, I've been failing to justify spending the time to working through those issues. There's definitely still some value here if we choose to revisit in the future, but for now I'll close this pull request.