From abde826e3c3c857b0ce65ff5d8c6d9922f9401f1 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Thu, 20 Apr 2023 12:51:05 +0200 Subject: [PATCH] Revert conditional selects from #47243 --- .../data/src/components/use-select/index.js | 142 +++++++----------- .../src/components/use-select/test/index.js | 102 +------------ 2 files changed, 64 insertions(+), 180 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index e1082b50a54657..d76d6ec354739a 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -42,85 +42,48 @@ 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. @@ -128,17 +91,7 @@ function Store( registry, suspense ) { 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. @@ -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; } @@ -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 }; }; } @@ -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; diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js index 93ebf800be0a84..34193e0c06da04 100644 --- a/packages/data/src/components/use-select/test/index.js +++ b/packages/data/src/components/use-select/test/index.js @@ -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' } ), }, @@ -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. @@ -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
{ data }
; - } ); - - render( - - - - ); - - 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, [] ); @@ -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' ); @@ -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' ); @@ -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' ); } ); @@ -1223,11 +1137,11 @@ describe( 'useSelect', () => { rerender( ); 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' ); } ); } );