From dde60a8523b500ff4558189acfc3ff24196f9e74 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 26 Feb 2018 11:37:14 -0500 Subject: [PATCH 1/3] Data: Fix issue with in-stack unsubscribe --- data/index.js | 8 +++++++- data/test/index.js | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/data/index.js b/data/index.js index 93fa8fa80c09a..abd3f94299bbc 100644 --- a/data/index.js +++ b/data/index.js @@ -28,7 +28,13 @@ let listeners = []; * Global listener called for each store's update. */ export function globalListener() { - listeners.forEach( listener => listener() ); + // Use for loop instead of Array#forEach, as it's possible a listener's + // behavior causes one further in the stack to be unsubscribed. The + // latter's callback should not be called, which requires monitoring + // changes to the array as they occur in iteration. + for ( let i = 0; i < listeners.length; i++ ) { + listeners[ i ](); + } } /** diff --git a/data/test/index.js b/data/test/index.js index 8b63e41f4e716..488e47e579dd0 100644 --- a/data/test/index.js +++ b/data/test/index.js @@ -209,6 +209,20 @@ describe( 'withDispatch', () => { } ); describe( 'subscribe', () => { + const unsubscribes = []; + afterEach( () => { + let unsubscribe; + while ( ( unsubscribe = unsubscribes.shift() ) ) { + unsubscribe(); + } + } ); + + function subscribeWithUnsubscribe( ...args ) { + const unsubscribe = subscribe( ...args ); + unsubscribes.push( unsubscribe ); + return unsubscribe; + } + it( 'registers multiple selectors to the public API', () => { let incrementedValue = null; const store = registerReducer( 'myAwesomeReducer', ( state = 0 ) => state + 1 ); @@ -233,6 +247,21 @@ describe( 'subscribe', () => { expect( incrementedValue ).toBe( 3 ); } ); + + it( 'avoids calling a later listener if unsubscribed during earlier callback', () => { + const store = registerReducer( 'myAwesomeReducer', ( state = 0 ) => state + 1 ); + const firstListener = jest.fn( () => { + secondUnsubscribe(); + } ); + const secondListener = jest.fn(); + + subscribeWithUnsubscribe( firstListener ); + const secondUnsubscribe = subscribeWithUnsubscribe( secondListener ); + + store.dispatch( { type: 'dummy' } ); + + expect( secondListener ).not.toHaveBeenCalled(); + } ); } ); describe( 'dispatch', () => { From ff8516baf8f58bd230c0e0f1c4e739422a0366be Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 27 Feb 2018 20:27:11 -0500 Subject: [PATCH 2/3] Data: Standardize on snapshotting behavior --- data/index.js | 8 +------- data/test/index.js | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/data/index.js b/data/index.js index abd3f94299bbc..fd2435e78e8b3 100644 --- a/data/index.js +++ b/data/index.js @@ -28,13 +28,7 @@ let listeners = []; * Global listener called for each store's update. */ export function globalListener() { - // Use for loop instead of Array#forEach, as it's possible a listener's - // behavior causes one further in the stack to be unsubscribed. The - // latter's callback should not be called, which requires monitoring - // changes to the array as they occur in iteration. - for ( let i = 0; i < listeners.length; i++ ) { - listeners[ i ](); - } + listeners.forEach( ( listener ) => listener() ); } /** diff --git a/data/test/index.js b/data/test/index.js index 488e47e579dd0..b89e8f87aa324 100644 --- a/data/test/index.js +++ b/data/test/index.js @@ -248,7 +248,21 @@ describe( 'subscribe', () => { expect( incrementedValue ).toBe( 3 ); } ); - it( 'avoids calling a later listener if unsubscribed during earlier callback', () => { + it( 'snapshots listeners on change, avoiding a later listener if subscribed during earlier callback', () => { + const store = registerReducer( 'myAwesomeReducer', ( state = 0 ) => state + 1 ); + const secondListener = jest.fn(); + const firstListener = jest.fn( () => { + subscribeWithUnsubscribe( secondListener ); + } ); + + subscribeWithUnsubscribe( firstListener ); + + store.dispatch( { type: 'dummy' } ); + + expect( secondListener ).not.toHaveBeenCalled(); + } ); + + it( 'snapshots listeners on change, calling a later listener even if unsubscribed during earlier callback', () => { const store = registerReducer( 'myAwesomeReducer', ( state = 0 ) => state + 1 ); const firstListener = jest.fn( () => { secondUnsubscribe(); @@ -260,7 +274,7 @@ describe( 'subscribe', () => { store.dispatch( { type: 'dummy' } ); - expect( secondListener ).not.toHaveBeenCalled(); + expect( secondListener ).toHaveBeenCalled(); } ); } ); From b8b5322520ab9970375545b7bb4b4bc0acdf2a80 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 27 Feb 2018 20:39:18 -0500 Subject: [PATCH 3/3] Data: Track unmount to avoid runSelection --- data/index.js | 10 ++++++++++ data/test/index.js | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/data/index.js b/data/index.js index fd2435e78e8b3..d7a92bfad06e0 100644 --- a/data/index.js +++ b/data/index.js @@ -163,6 +163,12 @@ export const withSelect = ( mapStateToProps ) => ( WrappedComponent ) => { componentWillUnmount() { this.unsubscribe(); + + // While above unsubscribe avoids future listener calls, callbacks + // are snapshotted before being invoked, so if unmounting occurs + // during a previous callback, we need to explicitly track and + // avoid the `runSelection` that is scheduled to occur. + this.isUnmounting = true; } subscribe() { @@ -170,6 +176,10 @@ export const withSelect = ( mapStateToProps ) => ( WrappedComponent ) => { } runSelection( props = this.props ) { + if ( this.isUnmounting ) { + return; + } + const newState = mapStateToProps( select, props ); if ( ! isEqualShallow( newState, this.state ) ) { this.setState( newState ); diff --git a/data/test/index.js b/data/test/index.js index b89e8f87aa324..5b47a1d01939b 100644 --- a/data/test/index.js +++ b/data/test/index.js @@ -66,6 +66,20 @@ describe( 'select', () => { } ); describe( 'withSelect', () => { + const unsubscribes = []; + afterEach( () => { + let unsubscribe; + while ( ( unsubscribe = unsubscribes.shift() ) ) { + unsubscribe(); + } + } ); + + function subscribeWithUnsubscribe( ...args ) { + const unsubscribe = subscribe( ...args ); + unsubscribes.push( unsubscribe ); + return unsubscribe; + } + it( 'passes the relevant data to the component', () => { registerReducer( 'reactReducer', () => ( { reactKey: 'reactState' } ) ); registerSelectors( 'reactReducer', { @@ -162,6 +176,36 @@ describe( 'withSelect', () => { wrapper.unmount(); } ); + + it( 'ensures component is still mounted before setting state', () => { + // This test verifies that even though unsubscribe doesn't take effect + // until after the current listener stack is called, we don't attempt + // to setState on an unmounting `withSelect` component. It will fail if + // an attempt is made to `setState` on an unmounted component. + const store = registerReducer( 'counter', ( state = 0, action ) => { + if ( action.type === 'increment' ) { + return state + 1; + } + + return state; + } ); + + registerSelectors( 'counter', { + getCount: ( state, offset ) => state + offset, + } ); + + subscribeWithUnsubscribe( () => { + wrapper.unmount(); + } ); + + const Component = withSelect( ( _select, ownProps ) => ( { + count: _select( 'counter' ).getCount( ownProps.offset ), + } ) )( ( props ) =>
{ props.count }
); + + const wrapper = mount( ); + + store.dispatch( { type: 'increment' } ); + } ); } ); describe( 'withDispatch', () => {