walmartlabs/thorax — pull request #217 by kpdecker (2013-07-17 18:33:57+00:00)
Title: View event cleanup

This PR implements option (1) outlined below. Moving discussion here.

------

Continuing the IMs from earlier re: on vs. listenTo:

on:
- Unbinds on destroy of self for DOM, model, and collection events
- Does not unbind on destroy of source for backbone events
- Does not unbind on destroy of dest for backbone events

listenTo:
- Unbinds on destroy of dest for events (is backbone only)
- Does not unbind on destroy of source

At minimum there are leaks here. It feels like destroy should go tactical and call `off()`. This would unbind the on:source and listenTo:source cases above. This seems pretty straightforward to me and I can't think of any reasons not to considering how cripple the view is after a destroy call.

That leaves the question of the on:dest case. This is basically this one:
this.foo.on('bar', fn, this);
vs.
this.listenTo(this.foo, 'bar', fn);

The two operate in a very similar manner except when it comes to cleanup. This exception was where I hit metal stumbling block as I had expected all `on` calls to cleanup automatically on destroy.

It seems like the options are:
1. Break from the backbone behavior and transparently map `on` calls that pass a context to something more like listenTo so cleanup is bidirectional. This way all events are cleaned in the same manner regardless of how bound. Downside: This might surprise people as it doesn't follow backbone exactly.

2. Highlight the distinction between listenTo and on and how they operate under Thorax. We would want to note that the {model: {}, collection: {}} forms operate differently than the {foo: fn} forms.

To me it comes down to a question of:
a. Are we ok deviating from Backbone while using the same function name? (probably)
b. Are there use cases where a user might want to send events from a destroyed view to another?
c. How apparent are the errors going to be for the users? Memory leaks?