search

AmpersandJS / ampersand-state

Core state management object.
MIT License
141 stars 75 forks source link

Fix stopListening when changing a prop of type "state" #235

Closed janpaul123 closed 8 years ago

janpaul123 commented 8 years ago

Ampersand used to just call this.stopListening on a property of type "state" whenever we changed it out for some other object. However, this also unbinds any other listeners on that object, unexpectedly.

Instead, we should keep a reference to the handler, which we do by changing _getEventBubblingHandler into _getCachedEventBubblingHandler and saving it for the propertyName. This way, we can unbind just the handler we just set, and leave other event listeners untouched.

janpaul123 commented 8 years ago

Note that the CI only fails because it seems to not run on Saucelabs because of credentials issues, the actual test I wrote for this passes. (And fails without the patch.)

wraithgar commented 8 years ago

Yep, this makes sense. Thanks for taking the time to make this PR it looks like it took a bit of time to work out and test.

+1 from me.

janpaul123 commented 8 years ago

@wraithgar Thanks! Anything else necessary before this can get merged (the build environment seems to need to get fixed). I'd love to be able to point to regular ampersand-state again instead of to my clone. :smiley:

wraithgar commented 8 years ago

I was giving this some time to see if we got any other feedback from another ampersand.js member. Looks like nobody had anything to add so I'll go ahead and merge this.

wraithgar commented 8 years ago

Just a heads up we'll probably wait to ship this w/ the next major versoin of ampersand-state (which should be very soon, this PR is in works and has lots of attention on it) https://github.com/AmpersandJS/ampersand-state/pull/236

wraithgar commented 8 years ago

Actually it'd be best for you and others if I released the patch with this fix so you could update your stuff now instead of coupling this bugfix w/ the update to lodash 4. I'll publish this now.

wraithgar commented 8 years ago

published as v5.0.1

janpaul123 commented 8 years ago

Thanks a lot @wraithgar! We also might want to bump the versions on ampersand-model and ampersand-view some time soon — in the latest version they're pointing to ampersand-state 4.x.

wraithgar commented 8 years ago

yeah looks like those PRs never got made. I should think that update will make it into the version bumps being done for the lodash updates, but if you want one in the interim feel free to make PRs for that separately.