From f4c156110a6664c8bf0bfa6b66d7088a7f58ee15 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 5 Nov 2020 18:32:53 +0800 Subject: [PATCH 01/17] Try only react to changes in the selected stores --- .../data/src/components/use-select/index.js | 26 ++++++++++++++++--- packages/data/src/redux-store/index.js | 2 +- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 11cd09d9cdab4..6c4b384cc7954 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -94,6 +94,15 @@ export default function useSelect( _mapSelect, deps ) { const latestMapOutputError = useRef(); const isMountedAndNotUnsubscribing = useRef(); + const registeredStores = useRef( [] ); + const trapSelect = useCallback( + ( storeKey ) => { + registeredStores.current.push( storeKey ); + return registry.select( storeKey ); + }, + [ registry ] + ); + let mapOutput; try { @@ -101,7 +110,7 @@ export default function useSelect( _mapSelect, deps ) { latestMapSelect.current !== mapSelect || latestMapOutputError.current ) { - mapOutput = mapSelect( registry.select, registry ); + mapOutput = mapSelect( trapSelect, registry ); } else { mapOutput = latestMapOutput.current; } @@ -120,6 +129,10 @@ export default function useSelect( _mapSelect, deps ) { } } + useIsomorphicLayoutEffect( () => { + registeredStores.current = []; + }, [ mapSelect ] ); + useIsomorphicLayoutEffect( () => { latestMapSelect.current = mapSelect; latestMapOutput.current = mapOutput; @@ -141,7 +154,7 @@ export default function useSelect( _mapSelect, deps ) { if ( isMountedAndNotUnsubscribing.current ) { try { const newMapOutput = latestMapSelect.current( - registry.select, + trapSelect, registry ); if ( @@ -165,13 +178,20 @@ export default function useSelect( _mapSelect, deps ) { onStoreChange(); } - const unsubscribe = registry.subscribe( () => { + const onChange = () => { if ( latestIsAsync.current ) { renderQueue.add( queueContext, onStoreChange ); } else { onStoreChange(); } + }; + + const unsubscribers = registeredStores.current.map( ( storeKey ) => { + return registry.stores[ storeKey ].subscribe( onChange ); } ); + const unsubscribe = () => { + unsubscribers.map( ( unsubscriber ) => unsubscriber() ); + }; return () => { isMountedAndNotUnsubscribing.current = false; diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 55117182ba78c..edfd3083712cc 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -136,7 +136,7 @@ export default function createReduxStore( key, options ) { store && ( ( listener ) => { let lastState = store.__unstableOriginalGetState(); - store.subscribe( () => { + return store.subscribe( () => { const state = store.__unstableOriginalGetState(); const hasChanged = state !== lastState; lastState = state; From 8ec1f7338df9d45896c91b531e2b8265afd7e7fa Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 5 Nov 2020 18:33:12 +0800 Subject: [PATCH 02/17] Add tests --- .../src/components/use-select/test/index.js | 85 +++++++++++++++++-- 1 file changed, 78 insertions(+), 7 deletions(-) diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js index 78a81ba3d4798..c774539e1fe7f 100644 --- a/packages/data/src/components/use-select/test/index.js +++ b/packages/data/src/components/use-select/test/index.js @@ -133,23 +133,22 @@ describe( 'useSelect', () => { const data = useSelect( mapSelectSpy, [] ); return
; }; - let subscribedSpy, TestComponent; + let TestComponent; const mapSelectSpy = jest.fn( ( select ) => select( 'testStore' ).testSelector() ); const selectorSpy = jest.fn(); - const subscribeCallback = ( subscription ) => { - subscribedSpy = subscription; - }; beforeEach( () => { registry.registerStore( 'testStore', { - reducer: () => null, + actions: { + forceUpdate: () => ( { type: 'FORCE_UPDATE' } ), + }, + reducer: ( state = {} ) => ( { ...state } ), selectors: { testSelector: selectorSpy, }, } ); - registry.subscribe = subscribeCallback; TestComponent = getComponent( mapSelectSpy ); } ); afterEach( () => { @@ -194,7 +193,7 @@ describe( 'useSelect', () => { // subscription which should in turn trigger a re-render. act( () => { selectorSpy.mockReturnValue( valueB ); - subscribedSpy(); + registry.dispatch( 'testStore' ).forceUpdate(); } ); expect( testInstance.findByType( 'div' ).props.data ).toEqual( valueB @@ -203,4 +202,76 @@ describe( 'useSelect', () => { } ); } ); + + it( 'uses memoized selector if dependent stores do not change', () => { + const storeConfig = { + actions: { + increment: () => ( { type: 'INCREMENT' } ), + }, + reducer: ( state, action ) => { + if ( ! state ) { + return { counter: 0 }; + } + if ( action?.type === 'INCREMENT' ) { + return { counter: state.counter + 1 }; + } + return state; + }, + selectors: { + getCounter: ( state ) => state.counter, + }, + }; + + registry.registerStore( 'store-1', storeConfig ); + registry.registerStore( 'store-2', storeConfig ); + + let renderer; + + const selectCount1 = jest.fn( ( select ) => + select( 'store-1' ).getCounter() + ); + const selectCount2 = jest.fn( ( select ) => + select( 'store-2' ).getCounter() + ); + + const TestComponent = jest.fn( () => { + const count1 = useSelect( selectCount1, [] ); + useSelect( selectCount2, [] ); + + return
; + } ); + + act( () => { + renderer = TestRenderer.create( + + + + ); + } ); + + const testInstance = renderer.root; + + expect( selectCount1 ).toHaveBeenCalledTimes( 2 ); + expect( selectCount2 ).toHaveBeenCalledTimes( 2 ); + expect( TestComponent ).toHaveBeenCalledTimes( 1 ); + expect( testInstance.findByType( 'div' ).props.data ).toBe( 0 ); + + act( () => { + registry.dispatch( 'store-2' ).increment(); + } ); + + expect( selectCount1 ).toHaveBeenCalledTimes( 2 ); + expect( selectCount2 ).toHaveBeenCalledTimes( 3 ); + expect( TestComponent ).toHaveBeenCalledTimes( 2 ); + expect( testInstance.findByType( 'div' ).props.data ).toBe( 0 ); + + act( () => { + registry.dispatch( 'store-1' ).increment(); + } ); + + expect( selectCount1 ).toHaveBeenCalledTimes( 3 ); + expect( selectCount2 ).toHaveBeenCalledTimes( 3 ); + expect( TestComponent ).toHaveBeenCalledTimes( 3 ); + expect( testInstance.findByType( 'div' ).props.data ).toBe( 1 ); + } ); } ); From e92b1568685f964181bb84a095cc5409312bcc73 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 5 Nov 2020 18:33:23 +0800 Subject: [PATCH 03/17] Update failed tests --- .../src/components/with-select/test/index.js | 2 +- .../test/listener-hooks.js | 93 ++++++++++++------- 2 files changed, 62 insertions(+), 33 deletions(-) diff --git a/packages/data/src/components/with-select/test/index.js b/packages/data/src/components/with-select/test/index.js index 26f521c99059b..5a455e142f9c9 100644 --- a/packages/data/src/components/with-select/test/index.js +++ b/packages/data/src/components/with-select/test/index.js @@ -670,7 +670,7 @@ describe( 'withSelect', () => { // - 1 on initial render // - 1 on effect before subscription set. // - 1 child subscription fires. - expect( childMapSelectToProps ).toHaveBeenCalledTimes( 3 ); + expect( childMapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( parentMapSelectToProps ).toHaveBeenCalledTimes( 4 ); expect( ChildOriginalComponent ).toHaveBeenCalledTimes( 1 ); expect( ParentOriginalComponent ).toHaveBeenCalledTimes( 2 ); diff --git a/packages/edit-post/src/components/editor-initialization/test/listener-hooks.js b/packages/edit-post/src/components/editor-initialization/test/listener-hooks.js index 853d267243189..df18799c1cfa8 100644 --- a/packages/edit-post/src/components/editor-initialization/test/listener-hooks.js +++ b/packages/edit-post/src/components/editor-initialization/test/listener-hooks.js @@ -6,7 +6,7 @@ import TestRenderer, { act } from 'react-test-renderer'; /** * WordPress dependencies */ -import { RegistryProvider } from '@wordpress/data'; +import { RegistryProvider, createRegistry } from '@wordpress/data'; /** * Internal dependencies @@ -18,42 +18,62 @@ import { import { STORE_NAME } from '../../../store/constants'; describe( 'listener hook tests', () => { + const storeConfig = { + actions: { + forceUpdate: jest.fn( () => ( { type: 'FORCE_UPDATE' } ) ), + }, + reducer: ( state = {}, action ) => + action.type === 'FORCE_UPDATE' ? { ...state } : state, + }; const mockStores = { 'core/block-editor': { - getBlockSelectionStart: jest.fn(), + ...storeConfig, + selectors: { + getBlockSelectionStart: jest.fn(), + }, }, 'core/editor': { - getCurrentPost: jest.fn(), + ...storeConfig, + selectors: { + getCurrentPost: jest.fn(), + }, }, 'core/viewport': { - isViewportMatch: jest.fn(), + ...storeConfig, + selectors: { + isViewportMatch: jest.fn(), + }, }, [ STORE_NAME ]: { - isEditorSidebarOpened: jest.fn(), - openGeneralSidebar: jest.fn(), - closeGeneralSidebar: jest.fn(), - getActiveGeneralSidebarName: jest.fn(), - }, - }; - let subscribeTrigger; - const registry = { - select: jest - .fn() - .mockImplementation( ( storeName ) => mockStores[ storeName ] ), - dispatch: jest - .fn() - .mockImplementation( ( storeName ) => mockStores[ storeName ] ), - subscribe: ( subscription ) => { - subscribeTrigger = subscription; + ...storeConfig, + actions: { + ...storeConfig.actions, + openGeneralSidebar: jest.fn( () => ( { + type: 'OPEN_GENERAL_SIDEBAR', + } ) ), + closeGeneralSidebar: jest.fn( () => ( { + type: 'CLOSE_GENERAL_SIDEBAR', + } ) ), + }, + selectors: { + isEditorSidebarOpened: jest.fn(), + getActiveGeneralSidebarName: jest.fn(), + }, }, }; + + let registry; + beforeEach( () => { + registry = createRegistry( mockStores ); + } ); + const setMockReturnValue = ( store, functionName, value ) => { - mockStores[ store ][ functionName ] = jest - .fn() - .mockReturnValue( value ); + mockStores[ store ].selectors[ functionName ].mockReturnValue( value ); }; const getSpyedFunction = ( store, functionName ) => - mockStores[ store ][ functionName ]; + mockStores[ store ].selectors[ functionName ]; + const getSpyedAction = ( store, actionName ) => + mockStores[ store ].actions[ actionName ]; const renderComponent = ( testedHook, id, renderer = null ) => { const TestComponent = ( { postId } ) => { testedHook( postId ); @@ -70,11 +90,13 @@ describe( 'listener hook tests', () => { }; afterEach( () => { Object.values( mockStores ).forEach( ( storeMocks ) => { - Object.values( storeMocks ).forEach( ( mock ) => { + Object.values( storeMocks.selectors ).forEach( ( mock ) => { + mock.mockClear(); + } ); + Object.values( storeMocks.actions || {} ).forEach( ( mock ) => { mock.mockClear(); } ); } ); - subscribeTrigger = undefined; } ); describe( 'useBlockSelectionListener', () => { it( 'does nothing when editor sidebar is not open', () => { @@ -86,7 +108,7 @@ describe( 'listener hook tests', () => { getSpyedFunction( STORE_NAME, 'isEditorSidebarOpened' ) ).toHaveBeenCalled(); expect( - getSpyedFunction( STORE_NAME, 'openGeneralSidebar' ) + getSpyedAction( STORE_NAME, 'openGeneralSidebar' ) ).toHaveBeenCalledTimes( 0 ); } ); it( 'opens block sidebar if block is selected', () => { @@ -100,7 +122,7 @@ describe( 'listener hook tests', () => { renderComponent( useBlockSelectionListener, 10 ); } ); expect( - getSpyedFunction( STORE_NAME, 'openGeneralSidebar' ) + getSpyedAction( STORE_NAME, 'openGeneralSidebar' ) ).toHaveBeenCalledWith( 'edit-post/block' ); } ); it( 'opens document sidebar if block is not selected', () => { @@ -114,7 +136,7 @@ describe( 'listener hook tests', () => { renderComponent( useBlockSelectionListener, 10 ); } ); expect( - getSpyedFunction( STORE_NAME, 'openGeneralSidebar' ) + getSpyedAction( STORE_NAME, 'openGeneralSidebar' ) ).toHaveBeenCalledWith( 'edit-post/document' ); } ); } ); @@ -149,6 +171,9 @@ describe( 'listener hook tests', () => { expect( setAttribute ).not.toHaveBeenCalled(); } ); it( 'only calls document query selector once across renders', () => { + setMockReturnValue( 'core/editor', 'getCurrentPost', { + link: 'foo', + } ); act( () => { const renderer = renderComponent( useUpdatePostLinkListener, @@ -158,7 +183,7 @@ describe( 'listener hook tests', () => { } ); expect( mockSelector ).toHaveBeenCalledTimes( 1 ); act( () => { - subscribeTrigger(); + registry.dispatch( 'core/editor' ).forceUpdate(); } ); expect( mockSelector ).toHaveBeenCalledTimes( 1 ); } ); @@ -169,8 +194,9 @@ describe( 'listener hook tests', () => { act( () => { renderComponent( useUpdatePostLinkListener, 10 ); } ); + expect( setAttribute ).toHaveBeenCalledTimes( 1 ); act( () => { - subscribeTrigger(); + registry.dispatch( 'core/editor' ).forceUpdate(); } ); expect( setAttribute ).toHaveBeenCalledTimes( 1 ); } ); @@ -181,11 +207,14 @@ describe( 'listener hook tests', () => { act( () => { renderComponent( useUpdatePostLinkListener, 10 ); } ); + expect( setAttribute ).toHaveBeenCalledTimes( 1 ); + expect( setAttribute ).toHaveBeenCalledWith( 'href', 'foo' ); + setMockReturnValue( 'core/editor', 'getCurrentPost', { link: 'bar', } ); act( () => { - subscribeTrigger(); + registry.dispatch( 'core/editor' ).forceUpdate(); } ); expect( setAttribute ).toHaveBeenCalledTimes( 2 ); expect( setAttribute ).toHaveBeenCalledWith( 'href', 'bar' ); From 91cec38d579cb853c7767465b7942e3a6ba9b873 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 5 Nov 2020 19:35:07 +0800 Subject: [PATCH 04/17] Add more tests and comments --- .../data/src/components/use-select/index.js | 9 +- .../src/components/use-select/test/index.js | 199 ++++++++++++++---- 2 files changed, 165 insertions(+), 43 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 6c4b384cc7954..824cae3b4c873 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -94,6 +94,8 @@ export default function useSelect( _mapSelect, deps ) { const latestMapOutputError = useRef(); const isMountedAndNotUnsubscribing = useRef(); + // Keep track of the stores being selected in the mapSelect function, + // and only subscribe to those stores later. const registeredStores = useRef( [] ); const trapSelect = useCallback( ( storeKey ) => { @@ -129,6 +131,7 @@ export default function useSelect( _mapSelect, deps ) { } } + // Reset the registered stores when mapSelect changes. In case it subscribes to different stores. useIsomorphicLayoutEffect( () => { registeredStores.current = []; }, [ mapSelect ] ); @@ -186,9 +189,9 @@ export default function useSelect( _mapSelect, deps ) { } }; - const unsubscribers = registeredStores.current.map( ( storeKey ) => { - return registry.stores[ storeKey ].subscribe( onChange ); - } ); + const unsubscribers = registeredStores.current.map( ( storeKey ) => + registry.stores[ storeKey ].subscribe( onChange ) + ); const unsubscribe = () => { unsubscribers.map( ( unsubscriber ) => unsubscriber() ); }; diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js index c774539e1fe7f..bcb481239dd24 100644 --- a/packages/data/src/components/use-select/test/index.js +++ b/packages/data/src/components/use-select/test/index.js @@ -3,6 +3,11 @@ */ import TestRenderer, { act } from 'react-test-renderer'; +/** + * WordPress dependencies + */ +import { useState } from '@wordpress/element'; + /** * Internal dependencies */ @@ -203,8 +208,8 @@ describe( 'useSelect', () => { ); } ); - it( 'uses memoized selector if dependent stores do not change', () => { - const storeConfig = { + describe( 're-calls the selector as minimal times as possible', () => { + const counterStore = { actions: { increment: () => ( { type: 'INCREMENT' } ), }, @@ -222,56 +227,170 @@ describe( 'useSelect', () => { }, }; - registry.registerStore( 'store-1', storeConfig ); - registry.registerStore( 'store-2', storeConfig ); + it( 'only calls the selectors it has selected', () => { + registry.registerStore( 'store-1', counterStore ); + registry.registerStore( 'store-2', counterStore ); - let renderer; + let renderer; - const selectCount1 = jest.fn( ( select ) => - select( 'store-1' ).getCounter() - ); - const selectCount2 = jest.fn( ( select ) => - select( 'store-2' ).getCounter() - ); + const selectCount1 = jest.fn(); + const selectCount2 = jest.fn(); - const TestComponent = jest.fn( () => { - const count1 = useSelect( selectCount1, [] ); - useSelect( selectCount2, [] ); + const TestComponent = jest.fn( () => { + const count1 = useSelect( + ( select ) => + selectCount1() || select( 'store-1' ).getCounter(), + [] + ); + useSelect( + ( select ) => + selectCount2() || select( 'store-2' ).getCounter(), + [] + ); - return
; - } ); + return
; + } ); - act( () => { - renderer = TestRenderer.create( - - - - ); - } ); + act( () => { + renderer = TestRenderer.create( + + + + ); + } ); - const testInstance = renderer.root; + const testInstance = renderer.root; - expect( selectCount1 ).toHaveBeenCalledTimes( 2 ); - expect( selectCount2 ).toHaveBeenCalledTimes( 2 ); - expect( TestComponent ).toHaveBeenCalledTimes( 1 ); - expect( testInstance.findByType( 'div' ).props.data ).toBe( 0 ); + expect( selectCount1 ).toHaveBeenCalledTimes( 2 ); + expect( selectCount2 ).toHaveBeenCalledTimes( 2 ); + expect( TestComponent ).toHaveBeenCalledTimes( 1 ); + expect( testInstance.findByType( 'div' ).props.data ).toBe( 0 ); - act( () => { - registry.dispatch( 'store-2' ).increment(); + act( () => { + registry.dispatch( 'store-2' ).increment(); + } ); + + expect( selectCount1 ).toHaveBeenCalledTimes( 2 ); + expect( selectCount2 ).toHaveBeenCalledTimes( 3 ); + expect( TestComponent ).toHaveBeenCalledTimes( 2 ); + expect( testInstance.findByType( 'div' ).props.data ).toBe( 0 ); + + act( () => { + registry.dispatch( 'store-1' ).increment(); + } ); + + expect( selectCount1 ).toHaveBeenCalledTimes( 3 ); + expect( selectCount2 ).toHaveBeenCalledTimes( 3 ); + expect( TestComponent ).toHaveBeenCalledTimes( 3 ); + expect( testInstance.findByType( 'div' ).props.data ).toBe( 1 ); } ); - expect( selectCount1 ).toHaveBeenCalledTimes( 2 ); - expect( selectCount2 ).toHaveBeenCalledTimes( 3 ); - expect( TestComponent ).toHaveBeenCalledTimes( 2 ); - expect( testInstance.findByType( 'div' ).props.data ).toBe( 0 ); + it( 'can subscribe to multiple stores at once', () => { + registry.registerStore( 'store-1', counterStore ); + registry.registerStore( 'store-2', counterStore ); + registry.registerStore( 'store-3', counterStore ); - act( () => { - registry.dispatch( 'store-1' ).increment(); + let renderer; + + const selectCount1And2 = jest.fn(); + + const TestComponent = jest.fn( () => { + const { count1, count2 } = useSelect( + ( select ) => + selectCount1And2() || { + count1: select( 'store-1' ).getCounter(), + count2: select( 'store-2' ).getCounter(), + }, + [] + ); + + return
; + } ); + + act( () => { + renderer = TestRenderer.create( + + + + ); + } ); + + const testInstance = renderer.root; + + expect( selectCount1And2 ).toHaveBeenCalledTimes( 2 ); + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + count1: 0, + count2: 0, + } ); + + act( () => { + registry.dispatch( 'store-2' ).increment(); + } ); + + expect( selectCount1And2 ).toHaveBeenCalledTimes( 3 ); + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + count1: 0, + count2: 1, + } ); + + act( () => { + registry.dispatch( 'store-3' ).increment(); + } ); + + expect( selectCount1And2 ).toHaveBeenCalledTimes( 3 ); + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + count1: 0, + count2: 1, + } ); } ); - expect( selectCount1 ).toHaveBeenCalledTimes( 3 ); - expect( selectCount2 ).toHaveBeenCalledTimes( 3 ); - expect( TestComponent ).toHaveBeenCalledTimes( 3 ); - expect( testInstance.findByType( 'div' ).props.data ).toBe( 1 ); + it( 're-calls the selector when deps changed', () => { + registry.registerStore( 'store-1', counterStore ); + registry.registerStore( 'store-2', counterStore ); + registry.registerStore( 'store-3', counterStore ); + + let renderer, dep, setDep; + const selectCount1AndDep = jest.fn(); + + const TestComponent = jest.fn( () => { + [ dep, setDep ] = useState( 0 ); + const state = useSelect( + ( select ) => + selectCount1AndDep() || { + count1: select( 'store-1' ).getCounter(), + dep, + }, + [ dep ] + ); + + return
; + } ); + + act( () => { + renderer = TestRenderer.create( + + + + ); + } ); + + const testInstance = renderer.root; + + expect( selectCount1AndDep ).toHaveBeenCalledTimes( 2 ); + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + count1: 0, + dep: 0, + } ); + + act( () => { + setDep( 1 ); + } ); + + expect( selectCount1AndDep ).toHaveBeenCalledTimes( 3 ); + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + count1: 0, + dep: 1, + } ); + } ); } ); } ); From 59e730defd4ce0af6180c1070c7af67d7d1a96da Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 5 Nov 2020 19:38:04 +0800 Subject: [PATCH 05/17] Update comments --- packages/data/src/components/with-select/test/index.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/data/src/components/with-select/test/index.js b/packages/data/src/components/with-select/test/index.js index 5a455e142f9c9..efb094b0b3159 100644 --- a/packages/data/src/components/with-select/test/index.js +++ b/packages/data/src/components/with-select/test/index.js @@ -666,10 +666,6 @@ describe( 'withSelect', () => { registry.dispatch( 'childRender' ).toggleRender(); } ); - // 3 times because - // - 1 on initial render - // - 1 on effect before subscription set. - // - 1 child subscription fires. expect( childMapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( parentMapSelectToProps ).toHaveBeenCalledTimes( 4 ); expect( ChildOriginalComponent ).toHaveBeenCalledTimes( 1 ); From c4d3f6a4f3e28190e176ed32ea8bdce8970fe912 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Fri, 6 Nov 2020 15:18:49 +0800 Subject: [PATCH 06/17] Fix registry selector --- .../data/src/components/use-select/index.js | 64 ++++++++++--------- .../src/components/use-select/test/index.js | 59 +++++++++++++++++ packages/data/src/registry.js | 10 +++ 3 files changed, 102 insertions(+), 31 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 824cae3b4c873..10e4de53dedde 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -96,12 +96,13 @@ export default function useSelect( _mapSelect, deps ) { // Keep track of the stores being selected in the mapSelect function, // and only subscribe to those stores later. - const registeredStores = useRef( [] ); + const listeningStores = useRef( [] ); const trapSelect = useCallback( - ( storeKey ) => { - registeredStores.current.push( storeKey ); - return registry.select( storeKey ); - }, + ( callback ) => + registry.__experimentalMarkListeningStores( + callback, + listeningStores + ), [ registry ] ); @@ -112,7 +113,9 @@ export default function useSelect( _mapSelect, deps ) { latestMapSelect.current !== mapSelect || latestMapOutputError.current ) { - mapOutput = mapSelect( trapSelect, registry ); + mapOutput = trapSelect( () => + mapSelect( registry.select, registry ) + ); } else { mapOutput = latestMapOutput.current; } @@ -131,11 +134,6 @@ export default function useSelect( _mapSelect, deps ) { } } - // Reset the registered stores when mapSelect changes. In case it subscribes to different stores. - useIsomorphicLayoutEffect( () => { - registeredStores.current = []; - }, [ mapSelect ] ); - useIsomorphicLayoutEffect( () => { latestMapSelect.current = mapSelect; latestMapOutput.current = mapOutput; @@ -152,27 +150,25 @@ export default function useSelect( _mapSelect, deps ) { } } ); - useIsomorphicLayoutEffect( () => { - const onStoreChange = () => { - if ( isMountedAndNotUnsubscribing.current ) { - try { - const newMapOutput = latestMapSelect.current( - trapSelect, - registry - ); - if ( - isShallowEqual( latestMapOutput.current, newMapOutput ) - ) { - return; - } - latestMapOutput.current = newMapOutput; - } catch ( error ) { - latestMapOutputError.current = error; + const onStoreChange = useCallback( () => { + if ( isMountedAndNotUnsubscribing.current ) { + try { + const newMapOutput = trapSelect( () => + latestMapSelect.current( registry.select, registry ) + ); + + if ( isShallowEqual( latestMapOutput.current, newMapOutput ) ) { + return; } - forceRender(); + latestMapOutput.current = newMapOutput; + } catch ( error ) { + latestMapOutputError.current = error; } - }; + forceRender(); + } + }, [ registry ] ); + useIsomorphicLayoutEffect( () => { // catch any possible state changes during mount before the subscription // could be set. if ( latestIsAsync.current ) { @@ -180,7 +176,9 @@ export default function useSelect( _mapSelect, deps ) { } else { onStoreChange(); } + }, [ onStoreChange ] ); + useIsomorphicLayoutEffect( () => { const onChange = () => { if ( latestIsAsync.current ) { renderQueue.add( queueContext, onStoreChange ); @@ -189,7 +187,9 @@ export default function useSelect( _mapSelect, deps ) { } }; - const unsubscribers = registeredStores.current.map( ( storeKey ) => + const unsubscribers = [ + ...listeningStores.current, + ].map( ( storeKey ) => registry.stores[ storeKey ].subscribe( onChange ) ); const unsubscribe = () => { @@ -200,8 +200,10 @@ export default function useSelect( _mapSelect, deps ) { isMountedAndNotUnsubscribing.current = false; unsubscribe(); renderQueue.flush( queueContext ); + // Reset the registered stores when mapSelect changes. In case it subscribes to different stores. + listeningStores.current = new Set(); }; - }, [ registry ] ); + }, [ registry, onStoreChange, mapSelect ] ); return mapOutput; } diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js index bcb481239dd24..358bb33dddefa 100644 --- a/packages/data/src/components/use-select/test/index.js +++ b/packages/data/src/components/use-select/test/index.js @@ -12,6 +12,7 @@ import { useState } from '@wordpress/element'; * Internal dependencies */ import { createRegistry } from '../../../registry'; +import { createRegistrySelector } from '../../../factory'; import { RegistryProvider } from '../../registry-provider'; import useSelect from '../index'; @@ -392,5 +393,63 @@ describe( 'useSelect', () => { dep: 1, } ); } ); + + it( 'handles registry selectors', () => { + const getCount1And2 = createRegistrySelector( + ( select ) => ( state ) => ( { + count1: state.counter, + count2: select( 'store-2' ).getCounter(), + } ) + ); + + registry.registerStore( 'store-1', { + ...counterStore, + selectors: { + ...counterStore.selectors, + getCount1And2, + }, + } ); + registry.registerStore( 'store-2', counterStore ); + + let renderer; + const selectCount1And2 = jest.fn(); + + const TestComponent = jest.fn( () => { + const state = useSelect( + ( select ) => + selectCount1And2() || + select( 'store-1' ).getCount1And2(), + [] + ); + + return
; + } ); + + act( () => { + renderer = TestRenderer.create( + + + + ); + } ); + + const testInstance = renderer.root; + + expect( selectCount1And2 ).toHaveBeenCalledTimes( 2 ); + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + count1: 0, + count2: 0, + } ); + + act( () => { + registry.dispatch( 'store-2' ).increment(); + } ); + + expect( selectCount1And2 ).toHaveBeenCalledTimes( 3 ); + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + count1: 0, + count2: 1, + } ); + } ); } ); } ); diff --git a/packages/data/src/registry.js b/packages/data/src/registry.js index 674227f131409..14849b96cfc5f 100644 --- a/packages/data/src/registry.js +++ b/packages/data/src/registry.js @@ -50,6 +50,7 @@ import createCoreDataStore from './store'; export function createRegistry( storeConfigs = {}, parent = null ) { const stores = {}; let listeners = []; + const __experimentalListeningStores = new Set(); /** * Global listener called for each store's update. @@ -85,6 +86,7 @@ export function createRegistry( storeConfigs = {}, parent = null ) { const storeName = isObject( storeNameOrDefinition ) ? storeNameOrDefinition.name : storeNameOrDefinition; + __experimentalListeningStores.add( storeName ); const store = stores[ storeName ]; if ( store ) { return store.getSelectors(); @@ -93,6 +95,13 @@ export function createRegistry( storeConfigs = {}, parent = null ) { return parent && parent.select( storeName ); } + function __experimentalMarkListeningStores( callback, ref ) { + __experimentalListeningStores.clear(); + const result = callback.call( this ); + ref.current = Array.from( __experimentalListeningStores ); + return result; + } + const getResolveSelectors = memize( ( selectors ) => { return mapValues( @@ -223,6 +232,7 @@ export function createRegistry( storeConfigs = {}, parent = null ) { dispatch, use, register, + __experimentalMarkListeningStores, }; /** From cece79c825cfab8e6f8c739fb07e2f10165bfc31 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Fri, 6 Nov 2020 15:32:46 +0800 Subject: [PATCH 07/17] Add tests for conditionals in selectors --- .../src/components/use-select/test/index.js | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js index 358bb33dddefa..e02e52cb861b1 100644 --- a/packages/data/src/components/use-select/test/index.js +++ b/packages/data/src/components/use-select/test/index.js @@ -6,7 +6,7 @@ import TestRenderer, { act } from 'react-test-renderer'; /** * WordPress dependencies */ -import { useState } from '@wordpress/element'; +import { useState, useReducer } from '@wordpress/element'; /** * Internal dependencies @@ -451,5 +451,63 @@ describe( 'useSelect', () => { count2: 1, } ); } ); + + it( 'handles conditional statements in selectors', () => { + registry.registerStore( 'store-1', counterStore ); + registry.registerStore( 'store-2', counterStore ); + + let renderer, shouldSelectCount1, toggle; + const selectCount1 = jest.fn(); + const selectCount2 = jest.fn(); + + const TestComponent = jest.fn( () => { + [ shouldSelectCount1, toggle ] = useReducer( + ( should ) => ! should, + false + ); + const state = useSelect( + ( select ) => { + if ( shouldSelectCount1 ) { + selectCount1(); + select( 'store-1' ).getCounter(); + return 'count1'; + } + + selectCount2(); + select( 'store-2' ).getCounter(); + return 'count2'; + }, + [ shouldSelectCount1 ] + ); + + return
; + } ); + + act( () => { + renderer = TestRenderer.create( + + + + ); + } ); + + const testInstance = renderer.root; + + expect( selectCount1 ).toHaveBeenCalledTimes( 0 ); + expect( selectCount2 ).toHaveBeenCalledTimes( 2 ); + expect( testInstance.findByType( 'div' ).props.data ).toBe( + 'count2' + ); + + act( () => { + toggle(); + } ); + + expect( selectCount1 ).toHaveBeenCalledTimes( 1 ); + expect( selectCount2 ).toHaveBeenCalledTimes( 2 ); + expect( testInstance.findByType( 'div' ).props.data ).toBe( + 'count1' + ); + } ); } ); } ); From 1660f6d55b2b042de43dcd57eb9dee01d7fb17ba Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Fri, 6 Nov 2020 21:26:20 +0800 Subject: [PATCH 08/17] Fix listening stores type --- packages/data/src/components/use-select/index.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 10e4de53dedde..e663163bac0c1 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -187,9 +187,7 @@ export default function useSelect( _mapSelect, deps ) { } }; - const unsubscribers = [ - ...listeningStores.current, - ].map( ( storeKey ) => + const unsubscribers = listeningStores.current.map( ( storeKey ) => registry.stores[ storeKey ].subscribe( onChange ) ); const unsubscribe = () => { @@ -201,7 +199,7 @@ export default function useSelect( _mapSelect, deps ) { unsubscribe(); renderQueue.flush( queueContext ); // Reset the registered stores when mapSelect changes. In case it subscribes to different stores. - listeningStores.current = new Set(); + listeningStores.current = []; }; }, [ registry, onStoreChange, mapSelect ] ); From dd8b21a8589e45a7b21827eda21fb53523e739b4 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Mon, 9 Nov 2020 14:47:58 +0800 Subject: [PATCH 09/17] Use a flag and skip subscribing to stores that don't exist --- .../data/src/components/use-select/index.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index e663163bac0c1..4d8ff5897c611 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -13,6 +13,7 @@ import { useCallback, useEffect, useReducer, + useMemo, } from '@wordpress/element'; import isShallowEqual from '@wordpress/is-shallow-equal'; @@ -106,6 +107,11 @@ export default function useSelect( _mapSelect, deps ) { [ registry ] ); + // Generate a "flag" for used in the effect dependency array. + // It's different than just using `mapSelect` since deps could be undefined, + // in that case, we would still want to memoize it. + const depsChangedFlag = useMemo( () => ( {} ), deps || [] ); + let mapOutput; try { @@ -187,9 +193,13 @@ export default function useSelect( _mapSelect, deps ) { } }; - const unsubscribers = listeningStores.current.map( ( storeKey ) => - registry.stores[ storeKey ].subscribe( onChange ) - ); + const unsubscribers = listeningStores.current + // The registry could be a sub-registry that doesn't have the store directly. + // Skip subscribing to the store that doesn't exist. + .filter( ( storeKey ) => storeKey in registry.stores ) + .map( ( storeKey ) => + registry.stores[ storeKey ].subscribe( onChange ) + ); const unsubscribe = () => { unsubscribers.map( ( unsubscriber ) => unsubscriber() ); }; @@ -201,7 +211,7 @@ export default function useSelect( _mapSelect, deps ) { // Reset the registered stores when mapSelect changes. In case it subscribes to different stores. listeningStores.current = []; }; - }, [ registry, onStoreChange, mapSelect ] ); + }, [ registry, onStoreChange, depsChangedFlag ] ); return mapOutput; } From e4ca1e2ad3b6e35951d2f5842d2b373036147a7e Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 10 Nov 2020 11:13:34 +0800 Subject: [PATCH 10/17] Fix selectors not being called on deps changed --- .../data/src/components/use-select/index.js | 2 -- .../src/components/use-select/test/index.js | 17 +++++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 4d8ff5897c611..6407b79b9d859 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -182,9 +182,7 @@ export default function useSelect( _mapSelect, deps ) { } else { onStoreChange(); } - }, [ onStoreChange ] ); - useIsomorphicLayoutEffect( () => { const onChange = () => { if ( latestIsAsync.current ) { renderQueue.add( queueContext, onStoreChange ); diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js index e02e52cb861b1..a4e3cea2ddc13 100644 --- a/packages/data/src/components/use-select/test/index.js +++ b/packages/data/src/components/use-select/test/index.js @@ -116,7 +116,6 @@ describe( 'useSelect', () => { } ); // rerender with dependency changed - // rerender with non dependency changed act( () => { renderer.update( @@ -126,7 +125,7 @@ describe( 'useSelect', () => { } ); expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 ); - expect( selectSpyBar ).toHaveBeenCalledTimes( 1 ); + expect( selectSpyBar ).toHaveBeenCalledTimes( 2 ); expect( TestComponent ).toHaveBeenCalledTimes( 3 ); // ensure expected state was rendered @@ -387,11 +386,21 @@ describe( 'useSelect', () => { setDep( 1 ); } ); - expect( selectCount1AndDep ).toHaveBeenCalledTimes( 3 ); + expect( selectCount1AndDep ).toHaveBeenCalledTimes( 4 ); expect( testInstance.findByType( 'div' ).props.data ).toEqual( { count1: 0, dep: 1, } ); + + act( () => { + registry.dispatch( 'store-1' ).increment(); + } ); + + expect( selectCount1AndDep ).toHaveBeenCalledTimes( 5 ); + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + count1: 1, + dep: 1, + } ); } ); it( 'handles registry selectors', () => { @@ -503,7 +512,7 @@ describe( 'useSelect', () => { toggle(); } ); - expect( selectCount1 ).toHaveBeenCalledTimes( 1 ); + expect( selectCount1 ).toHaveBeenCalledTimes( 2 ); expect( selectCount2 ).toHaveBeenCalledTimes( 2 ); expect( testInstance.findByType( 'div' ).props.data ).toBe( 'count1' From a309dc587085bc82d8c77fefad9092997d74a7b2 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 10 Nov 2020 13:17:10 +0800 Subject: [PATCH 11/17] Handles subscriptions to the parent's stores --- .../data/src/components/use-select/index.js | 26 ++++++++-- .../src/components/use-select/test/index.js | 47 +++++++++++++++++++ packages/data/src/registry.js | 1 + 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 6407b79b9d859..dbb703357a475 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -36,6 +36,23 @@ const useIsomorphicLayoutEffect = const renderQueue = createQueue(); +/** + * Traverse up the registries to find the registry that has the store registered. + * + * @param {Object} registry The registry obtained by `useRegistry`. + * @param {string} storeKey The store key name. + * @return {Object | null} The registered store or null if it doesn't exists in the registry tree. + */ +function getRegisteredStore( registry, storeKey ) { + if ( ! registry ) { + return null; + } else if ( storeKey in registry.stores ) { + return registry.stores[ storeKey ]; + } + + return getRegisteredStore( registry.parent, storeKey ); +} + /** * Custom react hook for retrieving props from registered selectors. * @@ -192,11 +209,10 @@ export default function useSelect( _mapSelect, deps ) { }; const unsubscribers = listeningStores.current - // The registry could be a sub-registry that doesn't have the store directly. - // Skip subscribing to the store that doesn't exist. - .filter( ( storeKey ) => storeKey in registry.stores ) - .map( ( storeKey ) => - registry.stores[ storeKey ].subscribe( onChange ) + .map( ( storeKey ) => getRegisteredStore( registry, storeKey ) ) + .filter( ( registeredStore ) => registeredStore !== null ) + .map( ( registeredStore ) => + registeredStore.subscribe( onChange ) ); const unsubscribe = () => { unsubscribers.map( ( unsubscriber ) => unsubscriber() ); diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js index a4e3cea2ddc13..d5415191ffcdd 100644 --- a/packages/data/src/components/use-select/test/index.js +++ b/packages/data/src/components/use-select/test/index.js @@ -518,5 +518,52 @@ describe( 'useSelect', () => { 'count1' ); } ); + + it( "handles subscriptions to the parent's stores", () => { + registry.registerStore( 'parent-store', counterStore ); + + const subRegistry = createRegistry( {}, registry ); + subRegistry.registerStore( 'child-store', counterStore ); + + let renderer; + + const TestComponent = jest.fn( () => { + const state = useSelect( + ( select ) => ( { + parentCount: select( 'parent-store' ).getCounter(), + childCount: select( 'child-store' ).getCounter(), + } ), + [] + ); + + return
; + } ); + + act( () => { + renderer = TestRenderer.create( + + + + + + ); + } ); + + const testInstance = renderer.root; + + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + parentCount: 0, + childCount: 0, + } ); + + act( () => { + registry.dispatch( 'parent-store' ).increment(); + } ); + + expect( testInstance.findByType( 'div' ).props.data ).toEqual( { + parentCount: 1, + childCount: 0, + } ); + } ); } ); } ); diff --git a/packages/data/src/registry.js b/packages/data/src/registry.js index 14849b96cfc5f..9948daa25b73d 100644 --- a/packages/data/src/registry.js +++ b/packages/data/src/registry.js @@ -223,6 +223,7 @@ export function createRegistry( storeConfigs = {}, parent = null ) { } let registry = { + parent, registerGenericStore, stores, namespaces: stores, // TODO: Deprecate/remove this. From d8d7176d1a11062b4bdeba2efe62b31d4e189156 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 10 Nov 2020 14:46:45 +0800 Subject: [PATCH 12/17] Fix registry test --- packages/data/src/test/registry.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/data/src/test/registry.js b/packages/data/src/test/registry.js index c36bbe9865418..89c33fd2ce1cb 100644 --- a/packages/data/src/test/registry.js +++ b/packages/data/src/test/registry.js @@ -736,6 +736,9 @@ describe( 'createRegistry', () => { if ( key === 'stores' ) { return expect.any( Object ); } + if ( key === 'parent' ) { + return expect.any( Object ) || null; + } // TODO: Remove this after namsespaces is removed. if ( key === 'namespaces' ) { return registry.stores; From e74aba7b7df39e046926d7eec65296a792d9e22b Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 10 Nov 2020 16:02:11 +0800 Subject: [PATCH 13/17] Move onStoreChange back inside effect --- .../data/src/components/use-select/index.js | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index dbb703357a475..1f47aec03dbb9 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -173,25 +173,27 @@ export default function useSelect( _mapSelect, deps ) { } } ); - const onStoreChange = useCallback( () => { - if ( isMountedAndNotUnsubscribing.current ) { - try { - const newMapOutput = trapSelect( () => - latestMapSelect.current( registry.select, registry ) - ); - - if ( isShallowEqual( latestMapOutput.current, newMapOutput ) ) { - return; + useIsomorphicLayoutEffect( () => { + const onStoreChange = () => { + if ( isMountedAndNotUnsubscribing.current ) { + try { + const newMapOutput = trapSelect( () => + latestMapSelect.current( registry.select, registry ) + ); + + if ( + isShallowEqual( latestMapOutput.current, newMapOutput ) + ) { + return; + } + latestMapOutput.current = newMapOutput; + } catch ( error ) { + latestMapOutputError.current = error; } - latestMapOutput.current = newMapOutput; - } catch ( error ) { - latestMapOutputError.current = error; + forceRender(); } - forceRender(); - } - }, [ registry ] ); + }; - useIsomorphicLayoutEffect( () => { // catch any possible state changes during mount before the subscription // could be set. if ( latestIsAsync.current ) { @@ -225,7 +227,7 @@ export default function useSelect( _mapSelect, deps ) { // Reset the registered stores when mapSelect changes. In case it subscribes to different stores. listeningStores.current = []; }; - }, [ registry, onStoreChange, depsChangedFlag ] ); + }, [ registry, trapSelect, depsChangedFlag ] ); return mapOutput; } From 0ce82715cbd3935d407ca5a090aca23cb1b1761f Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Wed, 11 Nov 2020 15:26:12 +0800 Subject: [PATCH 14/17] Update comment --- packages/data/src/components/use-select/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 1f47aec03dbb9..7e9eef8f6e025 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -224,7 +224,7 @@ export default function useSelect( _mapSelect, deps ) { isMountedAndNotUnsubscribing.current = false; unsubscribe(); renderQueue.flush( queueContext ); - // Reset the registered stores when mapSelect changes. In case it subscribes to different stores. + // Reset the registered stores when depsChangedFlag changes. In case it subscribes to different stores. listeningStores.current = []; }; }, [ registry, trapSelect, depsChangedFlag ] ); From 9295f6dd15f70e0a90260840898f9d0f61cb967a Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 26 Nov 2020 15:39:11 +0800 Subject: [PATCH 15/17] Use a weak map to record stores/parent privately --- .../data/src/components/use-select/index.js | 32 ++--------- packages/data/src/registry.js | 57 ++++++++++++++++++- packages/data/src/test/registry.js | 3 - 3 files changed, 59 insertions(+), 33 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 7e9eef8f6e025..782015355527a 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -36,23 +36,6 @@ const useIsomorphicLayoutEffect = const renderQueue = createQueue(); -/** - * Traverse up the registries to find the registry that has the store registered. - * - * @param {Object} registry The registry obtained by `useRegistry`. - * @param {string} storeKey The store key name. - * @return {Object | null} The registered store or null if it doesn't exists in the registry tree. - */ -function getRegisteredStore( registry, storeKey ) { - if ( ! registry ) { - return null; - } else if ( storeKey in registry.stores ) { - return registry.stores[ storeKey ]; - } - - return getRegisteredStore( registry.parent, storeKey ); -} - /** * Custom react hook for retrieving props from registered selectors. * @@ -210,22 +193,15 @@ export default function useSelect( _mapSelect, deps ) { } }; - const unsubscribers = listeningStores.current - .map( ( storeKey ) => getRegisteredStore( registry, storeKey ) ) - .filter( ( registeredStore ) => registeredStore !== null ) - .map( ( registeredStore ) => - registeredStore.subscribe( onChange ) - ); - const unsubscribe = () => { - unsubscribers.map( ( unsubscriber ) => unsubscriber() ); - }; + const unsubscribe = registry.__experimentalSubscribeStores( + listeningStores.current, + onChange + ); return () => { isMountedAndNotUnsubscribing.current = false; unsubscribe(); renderQueue.flush( queueContext ); - // Reset the registered stores when depsChangedFlag changes. In case it subscribes to different stores. - listeningStores.current = []; }; }, [ registry, trapSelect, depsChangedFlag ] ); diff --git a/packages/data/src/registry.js b/packages/data/src/registry.js index 9948daa25b73d..a5697290d6d88 100644 --- a/packages/data/src/registry.js +++ b/packages/data/src/registry.js @@ -12,6 +12,30 @@ import createCoreDataStore from './store'; /** @typedef {import('./types').WPDataStore} WPDataStore */ +// A map to record each registry and their stores/parent privately. +const StoresMap = new WeakMap(); + +/** + * Traverse up the registries to find the registry that has the store registered. + * + * @param {Object} registry The registry created by createRegistry below. + * @param {string} storeName The store key name. + * @return {Object | null} The registered store or null if it doesn't exists in the registry tree. + */ +function getRegisteredStore( registry, storeName ) { + if ( ! registry ) { + return null; + } + + const { stores, parent } = StoresMap.get( registry ); + + if ( storeName in stores ) { + return stores[ storeName ]; + } + + return getRegisteredStore( parent, storeName ); +} + /** * @typedef {Object} WPDataRegistry An isolated orchestrator of store registrations. * @@ -222,8 +246,25 @@ export function createRegistry( storeConfigs = {}, parent = null ) { registerGenericStore( store.name, store.instantiate( registry ) ); } + /** + * Subscribe handler to all the stores. + * + * @param {string[]} listeningStores Store names in array. + * @param {Function} handler The function subscribed to the stores. + * @return {Function} A function to unsubscribe every handler. + */ + function __experimentalSubscribeStores( listeningStores, handler ) { + const unsubscribers = listeningStores + .map( ( storeName ) => getRegisteredStore( registry, storeName ) ) + .filter( ( registeredStore ) => registeredStore !== null ) + .map( ( registeredStore ) => registeredStore.subscribe( handler ) ); + + return () => { + unsubscribers.forEach( ( unsubscriber ) => unsubscriber() ); + }; + } + let registry = { - parent, registerGenericStore, stores, namespaces: stores, // TODO: Deprecate/remove this. @@ -234,6 +275,7 @@ export function createRegistry( storeConfigs = {}, parent = null ) { use, register, __experimentalMarkListeningStores, + __experimentalSubscribeStores, }; /** @@ -279,5 +321,16 @@ export function createRegistry( storeConfigs = {}, parent = null ) { parent.subscribe( globalListener ); } - return withPlugins( registry ); + const storesMap = { + stores, + parent, + }; + StoresMap.set( registry, storesMap ); + + const registryWithPlugins = withPlugins( registry ); + + // We don't need to do this once `withPlugins` is removed. + StoresMap.set( registryWithPlugins, storesMap ); + + return registryWithPlugins; } diff --git a/packages/data/src/test/registry.js b/packages/data/src/test/registry.js index 89c33fd2ce1cb..c36bbe9865418 100644 --- a/packages/data/src/test/registry.js +++ b/packages/data/src/test/registry.js @@ -736,9 +736,6 @@ describe( 'createRegistry', () => { if ( key === 'stores' ) { return expect.any( Object ); } - if ( key === 'parent' ) { - return expect.any( Object ) || null; - } // TODO: Remove this after namsespaces is removed. if ( key === 'namespaces' ) { return registry.stores; From e963136b95a735f73065d24384e5d4236842c8d3 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 26 Nov 2020 16:40:46 +0800 Subject: [PATCH 16/17] Handle use --- packages/data/src/registry.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/data/src/registry.js b/packages/data/src/registry.js index a5697290d6d88..12c4e6cdaf329 100644 --- a/packages/data/src/registry.js +++ b/packages/data/src/registry.js @@ -303,11 +303,16 @@ export function createRegistry( storeConfigs = {}, parent = null ) { // This function will be deprecated as soon as it is no longer internally referenced. // function use( plugin, options ) { + const storesMap = StoresMap.get( registry ); + registry = { ...registry, ...plugin( registry, options ), }; + // We don't need to do this once `use` is removed. + StoresMap.set( registry, storesMap ); + return registry; } From ecc8d741c1587ec7e435abc6b00603a6479a3518 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 26 Nov 2020 18:43:57 +0800 Subject: [PATCH 17/17] Subscribe store recursively --- .../data/src/components/use-select/index.js | 7 +- packages/data/src/registry.js | 65 ++++--------------- 2 files changed, 14 insertions(+), 58 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 782015355527a..f1443e842e4bf 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -193,14 +193,13 @@ export default function useSelect( _mapSelect, deps ) { } }; - const unsubscribe = registry.__experimentalSubscribeStores( - listeningStores.current, - onChange + const unsubscribers = listeningStores.current.map( ( storeName ) => + registry.__experimentalSubscribeStore( storeName, onChange ) ); return () => { isMountedAndNotUnsubscribing.current = false; - unsubscribe(); + unsubscribers.forEach( ( unsubscribe ) => unsubscribe() ); renderQueue.flush( queueContext ); }; }, [ registry, trapSelect, depsChangedFlag ] ); diff --git a/packages/data/src/registry.js b/packages/data/src/registry.js index 12c4e6cdaf329..9924896ef04d8 100644 --- a/packages/data/src/registry.js +++ b/packages/data/src/registry.js @@ -12,30 +12,6 @@ import createCoreDataStore from './store'; /** @typedef {import('./types').WPDataStore} WPDataStore */ -// A map to record each registry and their stores/parent privately. -const StoresMap = new WeakMap(); - -/** - * Traverse up the registries to find the registry that has the store registered. - * - * @param {Object} registry The registry created by createRegistry below. - * @param {string} storeName The store key name. - * @return {Object | null} The registered store or null if it doesn't exists in the registry tree. - */ -function getRegisteredStore( registry, storeName ) { - if ( ! registry ) { - return null; - } - - const { stores, parent } = StoresMap.get( registry ); - - if ( storeName in stores ) { - return stores[ storeName ]; - } - - return getRegisteredStore( parent, storeName ); -} - /** * @typedef {Object} WPDataRegistry An isolated orchestrator of store registrations. * @@ -247,21 +223,18 @@ export function createRegistry( storeConfigs = {}, parent = null ) { } /** - * Subscribe handler to all the stores. + * Subscribe handler to a store. * - * @param {string[]} listeningStores Store names in array. - * @param {Function} handler The function subscribed to the stores. - * @return {Function} A function to unsubscribe every handler. + * @param {string[]} storeName The store name. + * @param {Function} handler The function subscribed to the store. + * @return {Function} A function to unsubscribe the handler. */ - function __experimentalSubscribeStores( listeningStores, handler ) { - const unsubscribers = listeningStores - .map( ( storeName ) => getRegisteredStore( registry, storeName ) ) - .filter( ( registeredStore ) => registeredStore !== null ) - .map( ( registeredStore ) => registeredStore.subscribe( handler ) ); + function __experimentalSubscribeStore( storeName, handler ) { + if ( storeName in stores ) { + return stores[ storeName ].subscribe( handler ); + } - return () => { - unsubscribers.forEach( ( unsubscriber ) => unsubscriber() ); - }; + return parent.__experimentalSubscribeStore( storeName, handler ); } let registry = { @@ -275,7 +248,7 @@ export function createRegistry( storeConfigs = {}, parent = null ) { use, register, __experimentalMarkListeningStores, - __experimentalSubscribeStores, + __experimentalSubscribeStore, }; /** @@ -303,16 +276,11 @@ export function createRegistry( storeConfigs = {}, parent = null ) { // This function will be deprecated as soon as it is no longer internally referenced. // function use( plugin, options ) { - const storesMap = StoresMap.get( registry ); - registry = { ...registry, ...plugin( registry, options ), }; - // We don't need to do this once `use` is removed. - StoresMap.set( registry, storesMap ); - return registry; } @@ -326,16 +294,5 @@ export function createRegistry( storeConfigs = {}, parent = null ) { parent.subscribe( globalListener ); } - const storesMap = { - stores, - parent, - }; - StoresMap.set( registry, storesMap ); - - const registryWithPlugins = withPlugins( registry ); - - // We don't need to do this once `withPlugins` is removed. - StoresMap.set( registryWithPlugins, storesMap ); - - return registryWithPlugins; + return withPlugins( registry ); }