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.

Sign up for GitHub

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

Data: Fix issue with in-stack unsubscribe #5266

Merged
merged 3 commits into from
Feb 28, 2018
Merged

Data: Fix issue with in-stack unsubscribe #5266

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dde60a8
Data: Fix issue with in-stack unsubscribe
aduth Feb 26, 2018
ff8516b
Data: Standardize on snapshotting behavior
aduth Feb 28, 2018
b8b5322
Data: Track unmount to avoid runSelection
aduth Feb 28, 2018
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
8 changes: 7 additions & 1 deletion data/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++ ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a listener removes itself, I think the keys of the listener could change in a way, we'd miss one listener call right?
Also, I believe the listeners.length is performed once and thus listeners[ i ] could be undefined at some point? And if a listener is added in another listener, should we call these listeners :)

Ok my brain crashed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a listener removes itself, I think the keys of the listener could change in a way, we'd miss one listener call right?

May be! I'll add a test and check.

Also, I believe the listeners.length is performed once and thus listeners[ i ] could be undefined at some point?

I'm not sure this is true, but I'll have to check the specification. I know that this used to be an optimization that would be commonly recommended:

for ( var i = 0, len = listeners.length; i < len; i++ ) {}

...but that browsers have since largely internally optimized. The fact that it was ever suggested should imply that it's checked on each iteration (and that, if browsers optimize, they do so safely to still allow for the original specification behavior).

And if a listener is added in another listener, should we call these listeners :)

Considering at a high level? I'm not really sure. In practice, I expect it probably should be called in the current implementation, since it pushes a new listener into the stack. I guess prior to these changes it would not have happened.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I recalled this being internally handled in Redux as well:

https://github.com/reactjs/redux/blob/55e77e88c98723f1883929458bb0144430108143/src/createStore.js#L78-L138

I might review their implementation to see if we can mimic the same expectations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, on a cursory glance, that implementation would be represented with the current implementation prior to these proposed changes. But obviously we still have the issue of setState being called after unsubscribe. At worst, we could add some logic to see that the withSelect component is still mounted before calling this.setState.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we keep the previous implementation and just check that the listener is still in the array before calling it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the consistency in expected behavior of Redux:

If you subscribe or unsubscribe while the listeners are being invoked, this
will not have any effect on the dispatch() that is currently in progress.

I'd say that we don't need to adopt the same behavior, but we should aim for similar consistency. If removing a listener takes effect immediately, then so should adding a listener, and vice-versa.

The more I think about it, the more I think we should just adopt this same behavior and include a check from within withSelect to achieve the desired behavior.

i.e.

diff --git a/data/index.js b/data/index.js
index abd3f9429..ea38314e9 100644
--- a/data/index.js
+++ b/data/index.js
@@ -169,6 +169,8 @@ export const withSelect = ( mapStateToProps ) => ( WrappedComponent ) => {
 
 		componentWillUnmount() {
 			this.unsubscribe();
+
+			this.isUnmounting = true;
 		}
 
 		subscribe() {
@@ -176,6 +178,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 );

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍

listeners[ i ]();
}
}

/**
Expand Down
29 changes: 29 additions & 0 deletions data/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand All @@ -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', () => {
Expand Down