Data: Fix issue with in-stack unsubscribe

[aduth]

This pull request seeks to resolve an issue with the data module's unsubscribe behavior which can result in a listener callback being invoked even after its been unsubscribed. In practice, this has been observed in #5206 where a withSelect's setState would be called even after the component has unmounted. The solution is using a for loop in place of Array#forEach, where the latter does not account for modifications which occur to the underlying array while it is being iterated over. Since we might assume a listener's behavior could cause a later listener to become unsubscribed, we should support this by operating on the raw array, including modifications, while iterating.

Testing instructions:

Ensure unit tests pass:

npm run test-unit

[youknowriad]

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

[aduth]

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.

[aduth]

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.

[aduth]

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.

[youknowriad]

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

[aduth]

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?

[youknowriad]

Sounds good to me 👍

[aduth]

Updated to snapshotting behavior (with a couple new tests verifying said behavior) in ff8516b.

setState on unmount is avoided via instance property in b8b5322, with test and inline documentation.

[youknowriad]

LGTM 👍