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.

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

useSelect: only invalidate on subscribe if store changed #57108

Merged
merged 6 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 8 additions & 10 deletions packages/core-data/src/hooks/test/use-query-select.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,9 @@ describe( 'useQuerySelect', () => {
<TestComponent keyName="foo" />
</RegistryProvider>
);
// 2 times expected
// - 1 for initial mount
// - 1 for after mount before subscription set.
expect( selectSpy ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );

expect( selectSpy ).toHaveBeenCalledTimes( 1 );
expect( TestComponent ).toHaveBeenCalledTimes( 1 );

// ensure expected state was rendered
expect( screen.getByText( 'bar' ) ).toBeInTheDocument();
Expand All @@ -81,10 +79,9 @@ describe( 'useQuerySelect', () => {
);

// ensure the selectors were properly memoized
expect( selectors ).toHaveLength( 4 );
expect( selectors ).toHaveLength( 2 );
expect( selectors[ 0 ] ).toHaveProperty( 'testSelector' );
expect( selectors[ 0 ] ).toBe( selectors[ 1 ] );
expect( selectors[ 1 ] ).toBe( selectors[ 2 ] );

// Re-render
render(
Expand All @@ -94,9 +91,10 @@ describe( 'useQuerySelect', () => {
);

// ensure we still got the memoized results after re-rendering
expect( selectors ).toHaveLength( 8 );
expect( selectors[ 3 ] ).toHaveProperty( 'testSelector' );
expect( selectors[ 5 ] ).toBe( selectors[ 6 ] );
expect( selectors ).toHaveLength( 4 );
expect( selectors[ 2 ] ).toHaveProperty( 'testSelector' );
expect( selectors[ 1 ] ).toBe( selectors[ 2 ] );
expect( selectors[ 2 ] ).toBe( selectors[ 3 ] );
} );

it( 'returns the expected "response" details – no resolvers and arguments', () => {
Expand Down
35 changes: 29 additions & 6 deletions packages/data/src/components/use-select/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ function Store( registry, suspense ) {
let lastIsAsync;
let subscriber;
let didWarnUnstableReference;
const storeStatesOnMount = new Map();

function getStoreState( name ) {
// If there's no store property (custom generic store), return an empty
// object. When comparing the state, the empty objects will cause the
// equality check to fail, setting `lastMapResultValid` to false.
return registry.stores[ name ]?.store?.getState?.() ?? {};
}

const createSubscriber = ( stores ) => {
// The set of stores the `subscribe` function is supposed to subscribe to. Here it is
Expand All @@ -56,12 +64,24 @@ function Store( registry, suspense ) {
const activeSubscriptions = new Set();

function subscribe( listener ) {
// Invalidate the value right after subscription was created. React will
// call `getValue` after subscribing, to detect store updates that happened
// in the interval between the `getValue` call during render and creating
// the subscription, which is slightly delayed. We need to ensure that this
// second `getValue` call will compute a fresh value.
lastMapResultValid = false;
// Maybe invalidate the value right after subscription was created.
// React will call `getValue` after subscribing, to detect store
// updates that happened in the interval between the `getValue` call
// during render and creating the subscription, which is slightly
// delayed. We need to ensure that this second `getValue` call will
// compute a fresh value only if any of the store states have
// changed in the meantime.
if ( lastMapResultValid ) {
for ( const name of activeStores ) {
if (
storeStatesOnMount.get( name ) !== getStoreState( name )
) {
lastMapResultValid = false;
}
}
}

storeStatesOnMount.clear();

const onStoreChange = () => {
// Invalidate the value on store update, so that a fresh value is computed.
Expand Down Expand Up @@ -149,6 +169,9 @@ function Store( registry, suspense ) {
}

if ( ! subscriber ) {
for ( const name of listeningStores.current ) {
storeStatesOnMount.set( name, getStoreState( name ) );
}
subscriber = createSubscriber( listeningStores.current );
} else {
subscriber.updateStores( listeningStores.current );
Expand Down
Loading
Loading