Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Model instance unbind() behavior inconsistent #418

Closed
navitmccaffery opened this Issue · 19 comments

7 participants

@navitmccaffery

Most Spine objects have the standard Events behavior, bind('event', callback) and unbind('event', callback). Unbinding specific callbacks is important when dynamically creating/releasing controllers so that they are properly garbage collected.

The behavior of the Model instance's unbind(), which is to unbind all callbacks for all events on that model instance (ignoring arguments), is a little surprising. I would expect all events to be unbound only if calling it with no arguments, and wiping all bound events can cause unexpected behavior if you have modularized your controllers to only clean up their own artifacts and not touch other controllers' events.

I realize that instance-level events on models end up creating wrapper events on the parent model's class, but it seems like a good idea to keep those wrapper functions around so that calls to unbind() can work as they do throughout the rest of the app, without outside code having to worry about implementation details.

@aeischeid
Owner

thought we addressed this in #402

@navitmccaffery

No, Model's unbind() overrides the Events one and simply triggers an 'unbind' event.
https://github.com/spine/spine/blob/master/src/spine.coffee#L394

@navitmccaffery

Similar-sounding issue name, different problem :)

@cengebretson

Definitely seems like this would be a good feature to add so its consistant with how unbind works, and it I can see where the current implementation of blowing all events away could lead to some tough to find bugs/issues. At the very least should be able to unbind all callbacks to a specific trigger, that should be a pretty easy change.

  unbind: (events) ->
    @trigger('unbind', events)

Unbinding a specific callback for a trigger looks a little trickier. At first glance it looks like we would need some way of keeping track of the unbinder function wrapper that bind() creates. Perhaps the bind() method should return an object of both {binder, unbinder} and the user would need to keep track of it. But there might be better approaches out there....

@aeischeid
Owner

Ah, I see. I think you are right and this is an important feature to add. Probably good to document it too. We will probably put this on the roadmap regardless, but of course pull requests always welcome ;)

@adambiggs
Collaborator

+1

I don't even use instance events anymore for this reason.

Could we maybe attach instance callbacks directly to records in the model store?

@cengebretson

I was playing around with attaching the unbinder function as a property of the original callback function passed into bind(). Then within the Events.unbind method I can check to see if the unbinder property exists and if so if it matches the passed in callback. I think it might work, will try it out on a separate branch first and see how it goes...

@aeischeid
Owner

keep an eye on the unbinder branch

@aeischeid
Owner

well @cengebretson and I (mostly @cengebretson ) came up with something... comments? suggestions? d8e5d36

@cengebretson

I made a few more changes tonight in the unbinder branch and I think I have things pretty close to working. Feels like there are a few more tests to write to be 100% sure. But the bind/one/unbind instance methods should hopefully behave just like the class level versions (should accept multiple event names and return the this object)...

@aeischeid
Owner

Good work!

I was looking at backbones code as we were working on this. Going to have to study that some more. Their pub-sub implementation looks like it is pretty slim and powerful. Spine's is starting to feel just a bit bulky and complex and we haven't even gotten to things like stopListening and listenTo and listenToOnce which seem like they might be pretty cool features.

@navitmccaffery

Yes, very nice. Thanks for looking into this, and coming up with solutions so quickly!

@aeischeid I have been using backbone as well and really like that when using listenTo() in a controller, events are automatically unbound on release (like those that are in the events mapping), so you don't have to worry about unbinding to clean up.

@adambiggs
Collaborator

Just checked out the docs for Backbone's listenTo(). This would be a really nice addition to Spine.

Currently I have to do this in every controller:

class SomeController extends Spine.Controller

    constructor: ->
        super
        SomeModel.bind 'change', @someCallback

# ...

    release: =>
        SomeModel.unbind 'change', @someCallback
        super

Is there a better way to do this in Spine?

@cengebretson

Looks interesting, so is the basic idea then that release() on the controller will take care of the unbinding? So the controller code now becomes...

class SomeController extends Spine.Controller
    constructor: ->
        super
        @listenTo(SomeModel, 'change', @someCallback)

I don't think this would be too hard to add into spine, at least controllers it wouldn't be. Would this be something that other types of classes would make use of. For example one model listening to events from another?

@aeischeid
Owner

if we were to port backbones idea it looks like in addition to a _callbacks on an object you can have a _listeners so in the events module maybe something like

listenTo: (obj, ev, callback) ->
    listens = @hasOwnProperty('_listeners') and @_listeners or= {}
    for name in evs
      listens[name] or= []
      listens[name].push(callback)
    this

  listenToOnce: (object, ev, callback) ->
    @listenTo ev, ->
      @stopListening(ev, arguments.callee)
      callback.apply(this, arguments)

  stopListening: (obj, ev, callback) ->
    unless ev
      @_listeners = {}
      return this
     evs  = ev.split(' ')
    for name in evs
      list = @_listeners?[name]
      continue unless list
      unless callback
        delete @_listeners[name]
        continue
      for cb, i in list when (cb is callback)
        list = list.slice()
        list.splice(i, 1)
        @_listeners[name] = list
        break
    this

update: probably @_listeners should be obj._listeners

@jamiter

+1 For a listenTo implementation. (Shouldn't it be discussed in a separate issue?).

I know its an example but for people who copy pasted the above code: it's missing a evs = ev.split(' ') in the listenTo function.

@johanlunds

Tried out the unbinder-branch in our project, but found a bug with accidentally unbinding classlevel-callbacks. I think I reproduced the problem with a failing test, see johanlunds@fbe4629.

Not sure what the correct fix is. Can someone help?

@cengebretson
Owner

I think I have it fixed in the latest commit to the branch 5fcc89b. I do feel like I need to write a few more tests for this feature, lots of edge cases, I'll try to do that this week. Thanks again for trying it out and for providing a test, much appreciated.

@aeischeid aeischeid closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.