Skip to content

Commit

Permalink
Revert conditional selects from #47243
Browse files Browse the repository at this point in the history
  • Loading branch information
@jsnajdr
jsnajdr committed Apr 20, 2023
1 parent 2ddfd2a commit abde826
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 180 deletions.
142 changes: 56 additions & 86 deletions packages/data/src/components/use-select/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,103 +42,56 @@ function Store( registry, suspense ) {
let lastMapResult;
let lastMapResultValid = false;
let lastIsAsync;
let subscriber;

const createSubscriber = ( stores ) => {
// The set of stores the `subscribe` function is supposed to subscribe to. Here it is
// initialized, and then the `updateStores` function can add new stores to it.
const activeStores = [ ...stores ];

// The `subscribe` function, which is passed to the `useSyncExternalStore` hook, could
// be called multiple times to establish multiple subscriptions. That's why we need to
// keep a set of active subscriptions;
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.
let subscribe;

const createSubscriber = ( stores ) => ( 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;

const onStoreChange = () => {
// Invalidate the value on store update, so that a fresh value is computed.
lastMapResultValid = false;
listener();
};

const onStoreChange = () => {
// Invalidate the value on store update, so that a fresh value is computed.
lastMapResultValid = false;
listener();
};

const onChange = () => {
if ( lastIsAsync ) {
renderQueue.add( queueContext, onStoreChange );
} else {
onStoreChange();
}
};

const unsubs = [];
function subscribeStore( storeName ) {
unsubs.push( registry.subscribe( onChange, storeName ) );
}

for ( const storeName of activeStores ) {
subscribeStore( storeName );
const onChange = () => {
if ( lastIsAsync ) {
renderQueue.add( queueContext, onStoreChange );
} else {
onStoreChange();
}
};

activeSubscriptions.add( subscribeStore );

return () => {
activeSubscriptions.delete( subscribeStore );

for ( const unsub of unsubs.values() ) {
// The return value of the subscribe function could be undefined if the store is a custom generic store.
unsub?.();
}
// Cancel existing store updates that were already scheduled.
renderQueue.cancel( queueContext );
};
}

// Check if `newStores` contains some stores we're not subscribed to yet, and add them.
function updateStores( newStores ) {
for ( const newStore of newStores ) {
if ( activeStores.includes( newStore ) ) {
continue;
}

// New `subscribe` calls will subscribe to `newStore`, too.
activeStores.push( newStore );
const unsubs = stores.map( ( storeName ) => {
return registry.subscribe( onChange, storeName );
} );

// Add `newStore` to existing subscriptions.
for ( const subscription of activeSubscriptions ) {
subscription( newStore );
}
return () => {
// The return value of the subscribe function could be undefined if the store is a custom generic store.
for ( const unsub of unsubs ) {
unsub?.();
}
}

return { subscribe, updateStores };
// Cancel existing store updates that were already scheduled.
renderQueue.cancel( queueContext );
};
};

return ( mapSelect, isAsync ) => {
function updateValue() {
return ( mapSelect, resubscribe, isAsync ) => {
const selectValue = () => mapSelect( select, registry );

function updateValue( selectFromStore ) {
// If the last value is valid, and the `mapSelect` callback hasn't changed,
// then we can safely return the cached value. The value can change only on
// store update, and in that case value will be invalidated by the listener.
if ( lastMapResultValid && mapSelect === lastMapSelect ) {
return lastMapResult;
}

const listeningStores = { current: null };
const mapResult = registry.__unstableMarkListeningStores(
() => mapSelect( select, registry ),
listeningStores
);

if ( ! subscriber ) {
subscriber = createSubscriber( listeningStores.current );
} else {
subscriber.updateStores( listeningStores.current );
}
const mapResult = selectFromStore();

// If the new value is shallow-equal to the old one, keep the old one so
// that we don't trigger unwanted updates that do a `===` check.
Expand All @@ -151,7 +104,7 @@ function Store( registry, suspense ) {

function getValue() {
// Update the value in case it's been invalidated or `mapSelect` has changed.
updateValue();
updateValue( selectValue );
return lastMapResult;
}

Expand All @@ -163,12 +116,29 @@ function Store( registry, suspense ) {
renderQueue.cancel( queueContext );
}

updateValue();
// Either initialize the `subscribe` function, or create a new one if `mapSelect`
// changed and has dependencies.
// Usage without dependencies, `useSelect( ( s ) => { ... } )`, will subscribe
// only once, at mount, and won't resubscibe even if `mapSelect` changes.
if ( ! subscribe || ( resubscribe && mapSelect !== lastMapSelect ) ) {
// Find out what stores the `mapSelect` callback is selecting from and
// use that list to create subscriptions to specific stores.
const listeningStores = { current: null };
updateValue( () =>
registry.__unstableMarkListeningStores(
selectValue,
listeningStores
)
);
subscribe = createSubscriber( listeningStores.current );
} else {
updateValue( selectValue );
}

lastIsAsync = isAsync;

// Return a pair of functions that can be passed to `useSyncExternalStore`.
return { subscribe: subscriber.subscribe, getValue };
return { subscribe, getValue };
};
}

Expand All @@ -181,7 +151,7 @@ function useMappingSelect( suspense, mapSelect, deps ) {
const isAsync = useAsyncMode();
const store = useMemo( () => Store( registry, suspense ), [ registry ] );
const selector = useCallback( mapSelect, deps );
const { subscribe, getValue } = store( selector, isAsync );
const { subscribe, getValue } = store( selector, !! deps, isAsync );
const result = useSyncExternalStore( subscribe, getValue, getValue );
useDebugValue( result );
return result;
Expand Down
102 changes: 8 additions & 94 deletions packages/data/src/components/use-select/test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import {
} from '../../..';
import useSelect from '..';

function counterStore( initialCount = 0, step = 1 ) {
function counterStore( initialCount = 0 ) {
return {
reducer: ( state = initialCount, action ) =>
action.type === 'INC' ? state + step : state,
action.type === 'INC' ? state + 1 : state,
actions: {
inc: () => ( { type: 'INC' } ),
},
Expand Down Expand Up @@ -122,7 +122,7 @@ describe( 'useSelect', () => {
);

expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 );
expect( selectSpyBar ).toHaveBeenCalledTimes( 1 );
expect( selectSpyBar ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 3 );

// Ensure expected state was rendered.
Expand Down Expand Up @@ -193,81 +193,6 @@ describe( 'useSelect', () => {
expect( Child ).toHaveBeenCalledTimes( 1 );
} );

it( 'incrementally subscribes to newly selected stores', () => {
registry.registerStore( 'store-main', counterStore() );
registry.registerStore( 'store-even', counterStore( 0, 2 ) );
registry.registerStore( 'store-odd', counterStore( 1, 2 ) );

const mapSelect = jest.fn( ( select ) => {
const first = select( 'store-main' ).get();
// select from other stores depending on whether main value is even or odd
const secondStore = first % 2 === 1 ? 'store-odd' : 'store-even';
const second = select( secondStore ).get();
return first + ':' + second;
} );

const TestComponent = jest.fn( () => {
const data = useSelect( mapSelect, [] );
return <div role="status">{ data }</div>;
} );

render(
<RegistryProvider value={ registry }>
<TestComponent />
</RegistryProvider>
);

expect( mapSelect ).toHaveBeenCalledTimes( 2 );
expect( TestComponent ).toHaveBeenCalledTimes( 1 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0:0' );

// check that increment in store-even triggers a render
act( () => {
registry.dispatch( 'store-even' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 3 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0:2' );

// check that increment in store-odd doesn't trigger a render (not listening yet)
act( () => {
registry.dispatch( 'store-odd' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 3 );
expect( TestComponent ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '0:2' );

// check that increment in main store switches to store-odd
act( () => {
registry.dispatch( 'store-main' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 4 );
expect( TestComponent ).toHaveBeenCalledTimes( 3 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '1:3' );

// check that increment in store-odd triggers a render
act( () => {
registry.dispatch( 'store-odd' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 5 );
expect( TestComponent ).toHaveBeenCalledTimes( 4 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '1:5' );

// check that increment in store-even triggers a mapSelect call (still listening)
// but not a render (not used for selected value which doesn't change)
act( () => {
registry.dispatch( 'store-even' ).inc();
} );

expect( mapSelect ).toHaveBeenCalledTimes( 6 );
expect( TestComponent ).toHaveBeenCalledTimes( 4 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '1:5' );
} );

describe( 'rerenders as expected with various mapSelect return types', () => {
const getComponent = ( mapSelectSpy ) => () => {
const data = useSelect( mapSelectSpy, [] );
Expand Down Expand Up @@ -484,7 +409,7 @@ describe( 'useSelect', () => {
setDep( 1 );
} );

expect( selectCount1AndDep ).toHaveBeenCalledTimes( 3 );
expect( selectCount1AndDep ).toHaveBeenCalledTimes( 4 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count:0,dep:1'
);
Expand All @@ -493,7 +418,7 @@ describe( 'useSelect', () => {
registry.dispatch( 'store-1' ).inc();
} );

expect( selectCount1AndDep ).toHaveBeenCalledTimes( 4 );
expect( selectCount1AndDep ).toHaveBeenCalledTimes( 5 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count:1,dep:1'
);
Expand Down Expand Up @@ -698,21 +623,10 @@ describe( 'useSelect', () => {

act( () => screen.getByText( 'Toggle' ).click() );

expect( selectCount1 ).toHaveBeenCalledTimes( 1 );
expect( selectCount2 ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count1:0'
);

// Verify that the component subscribed to store-1 after selected from
act( () => {
registry.dispatch( 'store-1' ).inc();
} );

expect( selectCount1 ).toHaveBeenCalledTimes( 2 );
expect( selectCount2 ).toHaveBeenCalledTimes( 2 );
expect( screen.getByRole( 'status' ) ).toHaveTextContent(
'count1:1'
'count1:0'
);
} );

Expand Down Expand Up @@ -1223,11 +1137,11 @@ describe( 'useSelect', () => {
rerender( <App store="counter-2" /> );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '10' );

// update from counter-2 is processed because component has subscribed to counter-2
// update from counter-2 is ignored because component is subcribed only to counter-1
act( () => {
registry.dispatch( 'counter-2' ).inc();
} );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '11' );
expect( screen.getByRole( 'status' ) ).toHaveTextContent( '10' );
} );
} );

Expand Down

0 comments on commit abde826

Please sign in to comment.