Adjust notifyObservers iteration when unwatch function is called during iteration [Fixes #151]

[c-spencer]

Proposed fix for #151.

This PR takes the somewhat ugly approach of reading/modifying the notifyObservers iteration from within the unwatch function. This won't work if notify loops can be nested, but I don't think that is possible/allowed?

The cleaner approach of taking a copy of __observers to iterate would mean that observers later in the array that might be unsubscribed during the iteration would still receive a notification.

(Might cause conflict with merging #115 which touches the same code, but retains the same bug.)

[balloob]

This also means that new notifiers will be called if registered during
notify?

Making a copy would make more sense to me and is probably more predictable
behavior for new developers.

On Sat, Aug 15, 2015, 09:45 Chris Spencer notifications@github.com wrote:

Proposed fix for #151
#151.

This PR takes the somewhat ugly approach of reading/modifying the
notifyObservers iteration from within the unwatch function. This won't work
if notify loops can be nested, but I don't think that is possible/allowed?

The cleaner approach of taking a copy of __observers to iterate would
mean that observers later in the array that might be unsubscribed during
the iteration would still receive a notification.

(Might cause conflict with merging #115
#115 which touches the

same code, but retains the same bug.)

You can view, comment on, or merge this pull request online at:

#152
Commit Summary

• Adjust notifyObservers iteration when unwatch function is called
  during iteration [Fixes If an observer is removed during notify, then some observers will not be run. #151]

File Changes

• M src/change-observer.js
  https://github.com/optimizely/nuclear-js/pull/152/files#diff-0 (16)
• M tests/change-observer-tests.js
  https://github.com/optimizely/nuclear-js/pull/152/files#diff-1 (11)

Patch Links:

• https://github.com/optimizely/nuclear-js/pull/152.patch
• https://github.com/optimizely/nuclear-js/pull/152.diff

—
Reply to this email directly or view it on GitHub
#152.

[c-spencer]

The current behaviour is that up to n new notifiers are currently called when registered during a notify, where n is the number of observers removed during that notify. If there's a desired behaviour one way or the other it can be changed (alternative approach at bottom of comment).

Agreed on an array copy being nicer, however I don't think it gives predictable behaviour, as calling unregistered handlers could lead to very hard to track bugs due to dependency on observer ordering.

For instance when working with React, handlers a, b where a unmounts a component and b does something with that same component. If they skip an isMounted() check then certain component APIs will throw if b lacks an isMounted check. But it'll work fine whenever they're ordered b, a. (Handler a could be way up the component hierarchy than handler b, giving spooky-action-at-a-distance.) In development you'd just get a mounting error and fix it, but in production it could cause your application to break seemingly at random.

An alternative approach would be to .slice(0) the observers, and have unwatch functions set an unwatched property on the entry that the notifyObservers iteration can check for to avoid running unregistered handlers. I can raise that alternative PR if wanted.

[balloob]

What if the isMounted() check would be added to the react mixin?

[c-spencer]

I think the ReactMixin only calls setState which React recently made tolerant (well, exception -> warning) to being called on unmounted components. In any case, #153 is probably a simple more palatable fix which also does not call new notifiers.

[jordangarcia]

This is fixed in #153