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.

Sign up for GitHub

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

Data: Provide dependencies from withSelect to useSelect #19007

Closed
wants to merge 3 commits into from
Closed

Data: Provide dependencies from withSelect to useSelect #19007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
10d6831
Data: Provide (empty) dependencies to useSelect hook
aduth Dec 9, 2019
7c8325d
Data: Pass ownProps dependency to useSelect
aduth Dec 9, 2019
f43b08d
Data: Update withSelect tests to account for memoized useSelect
aduth Dec 13, 2019
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
2 changes: 1 addition & 1 deletion packages/data/src/components/with-select/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent(
ownProps,
registry
);
const mergeProps = useSelect( mapSelect );
const mergeProps = useSelect( mapSelect, [ ownProps ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

return <WrappedComponent { ...ownProps } { ...mergeProps } />;
}
),
Expand Down
24 changes: 10 additions & 14 deletions packages/data/src/components/with-select/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,7 @@ describe( 'withSelect', () => {
// - 1 on initial render
// - 1 on effect before subscription set.
// - 1 on click triggering subscription firing.
// - 1 on rerender.
expect( mapSelectToProps ).toHaveBeenCalledTimes( 4 );
expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 );
// verifies component only renders twice.
expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );
} );
Expand Down Expand Up @@ -206,11 +205,10 @@ describe( 'withSelect', () => {
testInstance = testRenderer.root;
it( 'should rerun if had dispatched action during mount', () => {
expect( testInstance.findByType( 'div' ).props.children ).toBe( 2 );
// Expected 3 times because:
// Expected 2 times because:
// - 1 on initial render
// - 1 on effect before subscription set.
// - 1 for the rerender because of the mapOutput change detected.
expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 );
// - 1 on effect before subscription set, accounting for dispatch.
expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 );
expect( renderSpy ).toHaveBeenCalledTimes( 2 );
} );
it( 'should rerun on unmount and mount', () => {
Expand All @@ -220,11 +218,10 @@ describe( 'withSelect', () => {
} );
testInstance = testRenderer.root;
expect( testInstance.findByType( 'div' ).props.children ).toBe( 4 );
// Expected an additional 3 times because of the unmount and remount:
// Expected an additional 2 times because of the unmount and remount:
// - 1 on initial render
// - 1 on effect before subscription set.
// - once for the rerender because of the mapOutput change detected.
expect( mapSelectToProps ).toHaveBeenCalledTimes( 6 );
expect( mapSelectToProps ).toHaveBeenCalledTimes( 4 );
expect( renderSpy ).toHaveBeenCalledTimes( 4 );
} );
} );
Expand Down Expand Up @@ -612,7 +609,7 @@ describe( 'withSelect', () => {
// - 1 on effect before subscription set.
// - 1 child subscription fires.
expect( childMapSelectToProps ).toHaveBeenCalledTimes( 3 );
expect( parentMapSelectToProps ).toHaveBeenCalledTimes( 4 );
expect( parentMapSelectToProps ).toHaveBeenCalledTimes( 3 );
expect( ChildOriginalComponent ).toHaveBeenCalledTimes( 1 );
expect( ParentOriginalComponent ).toHaveBeenCalledTimes( 2 );
} );
Expand Down Expand Up @@ -671,13 +668,12 @@ describe( 'withSelect', () => {
</RegistryProvider>
);
} );
// 4 times:
// 3 times:
// - 1 on initial render
// - 1 on effect before subscription set.
// - 1 on re-render
// - 1 on effect before new subscription set (because registry has changed)
expect( mapSelectToProps ).toHaveBeenCalledTimes( 4 );
expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );
expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 );
expect( OriginalComponent ).toHaveBeenCalledTimes( 3 );
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was only called once before.

Copy link
Contributor

Choose a reason for hiding this comment

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

The mock count carries on from the previous render.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm referring to the change of:

Before:

expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );

After:

expect( OriginalComponent ).toHaveBeenCalledTimes( 3 );

Copy link
Contributor

Choose a reason for hiding this comment

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

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


expect( testInstance.findByType( 'div' ).props ).toEqual( {
children: 'second',
Expand Down