Added Event Cleanup Helpers #5

Closed
wants to merge 16 commits into
from

Projects

None yet

3 participants

@amkirwan
Contributor

Added some helpers for event cleanup when the view is removed. These are taken from the book and put into a mixin Support.Observer that is mixed into the Support.CompositeView. I have also added a callback function onLeave which will be called from the view if the user has implemented it before the view is removed from the DOM.

@jferris jferris and 1 other commented on an outdated diff Jul 20, 2012
...assets/javascripts/backbone-support/composite_view.js
@@ -5,6 +5,10 @@ Support.CompositeView = function(options) {
_.extend(Support.CompositeView.prototype, Backbone.View.prototype, {
leave: function() {
+ if (this.onLeave) {
@jferris
jferris Jul 20, 2012 Member

Using a Template Method like this doesn't feel natural in the Backbone world of events. I think having a "leave" event to bind to would be more natural.

@amkirwan
amkirwan Jul 20, 2012 Contributor

sounds good. I'll make the change.

@jferris jferris and 1 other commented on an outdated diff Jul 20, 2012
spec/javascripts/swapping_router_spec.js
@@ -75,7 +77,8 @@ describe("Support.SwappingRouter", function() {
it("renders and swaps backbone views", function() {
runs(function() {
- window.location.hash = "#red"
+ //window.location.hash = "#red"
@jferris
jferris Jul 20, 2012 Member

Can you remove this commented out line?

@amkirwan
amkirwan Jul 20, 2012 Contributor

sure will do

@jferris jferris commented on the diff Jul 20, 2012
spec/javascripts/composite_view_spec.js
+
+ runs(function() {
+ expect($("#test").text()).toEqual("");
+ expect(spy.called).toBeTruthy();
+ });
+ });
+ });
+
+ describe("#bindTo", function() {
+ var view = new orangeView();
+ var callback = sinon.spy();
+ var source = new Backbone.Model({
+ title: 'Model or Collection'
+ });
+
+ it("calls the unbindFromAll method when leaving the view", function() {
@jferris
jferris Jul 20, 2012 Member

This doesn't actually test that it ever binds to the event, does it? If you comment out line 3 of observer.js, do all the specs pass? This applies to the unbindFromAll spec below, as well as the specs for Observer.

@amkirwan
amkirwan Jul 20, 2012 Contributor

If you comment out line 3 of observer.js the observer_spec.js will fail because the test on line 45 checks if bind is called on the source object. The #unbindFromAll test will pass though because I don't test if bind is called. My thoughts were that this was something more in the domain of the observer_spec than the composite_view_spec but I can add a test to #unbindFromAll to check that bind is called on the source object if you think it needs one.

@jferris
Member
jferris commented Jul 20, 2012

Thanks for putting this together. This is a good change overall, but I had a few comments on the diff. I'll go over this patch again once you've had a chance to look those over.

@amkirwan
Contributor

I made a few changes to the code per your suggestions and added them to the branch.

@jferris
Member
jferris commented Jul 23, 2012

@amkirwan looks good. I'd like to rename onLeave to just leave to be more in line with existing Backbone events (change, error, etc). You can do that if you want, or I'll do it myself when I merge the branch in a bit. Thanks again for the patch!

@amkirwan
Contributor

@jferris I agree the event makes more sense as 'leave' than 'onLeave' for Backbone. Thanks for your help.

@lokimeyburg

I'm really liking these helpers! I'm using them right now.
Let me know if you need any help.
I'd like to see this merged.

@jferris jferris and 1 other commented on an outdated diff Nov 9, 2012
spec/javascripts/composite_view_spec.js
@@ -164,4 +200,80 @@ describe("Support.CompositeView", function() {
expect(view.children.size()).toEqual(1);
});
});
+
+ /*
@jferris
jferris Nov 9, 2012 Member

Just noticed that this spec is commented out. Should it be removed or uncommented?

@amkirwan
amkirwan Nov 9, 2012 Contributor

my bad this needs to be removed. That was from before we switched to using a leave event.

@jferris jferris and 1 other commented on an outdated diff Nov 9, 2012
lib/assets/javascripts/backbone-support/observer.js
+Support.Observer = {
+ bindTo: function(source, event, callback) {
+ source.bind(event, callback, this);
+ this.bindings = this.bindings || [];
+ this.bindings.push({ source: source, event: event, callback: callback });
+ },
+
+ unbindFromAll: function() {
+ _.each(this.bindings, function(binding) {
+ binding.source.unbind(binding.event, binding.callback);
+ });
+ this.bindings = []
+ }
+};
+
+ _.extend(Support.CompositeView.prototype, Support.Observer);
@jferris
jferris Nov 9, 2012 Member

Can we just add Support.Observer to the extend line in composite_view.js instead?

@amkirwan
amkirwan Nov 9, 2012 Contributor

We can do that the only downside is that I have to change the jasmine.yml and backbone-support.js to make sure observer.js is loaded first. If that sounds better to you than have the _.extend in observer.js I'll make the changes.

@jferris
jferris Nov 9, 2012 Member

Yeah, I think that would be better.

@jferris jferris commented on the diff Nov 9, 2012
spec/javascripts/swapping_router_spec.js
@@ -4,12 +4,14 @@ describe("Support.SwappingRouter", function() {
var redView = Backbone.View.extend({
@jferris
jferris Nov 9, 2012 Member

Why were the changes in this file necessary?

@amkirwan
amkirwan Nov 9, 2012 Contributor

I can't remember now why I used router.navigate. Maybe there was a reason or I was testing to make sure it all worked when using the backbone router. The spec passes either way I can switch it back to using the window.location.

@jferris
Member
jferris commented Nov 9, 2012

Sorry for the delay on this. I went through the pull request again and had a few more comments.

@amkirwan
Contributor

@jferris let me know if you need anything else to merge the changes

@jferris
Member
jferris commented Nov 21, 2012

Thanks. I merged this into master.

@jferris jferris closed this Nov 21, 2012
@lokimeyburg

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment