Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

View events removed on extend #244

Closed
mataspetrikas opened this issue Feb 23, 2011 · 12 comments
Closed

View events removed on extend #244

mataspetrikas opened this issue Feb 23, 2011 · 12 comments

Comments

@mataspetrikas
Copy link

when extending a View which has events defined, the original events get overwritten:
https://gist.github.com/840452
in example the, the onFooClick won't get triggered.
The problem is connected to the use of _.extend if I'm not mistaken.
what are the perspectives on implementing 'deep' merge/extend in general in Backbone?

@juggy
Copy link

juggy commented Mar 19, 2011

The nature of extend does not allow this. It will overwrite anything with the same name (it is basically a hash merge).

In your second view, add an event to events in the initialize function and call this.delegatEvents afterward. That should do it.

@mataspetrikas
Copy link
Author

@juggy: this is not an answer :) The workaround is obvious, but ugly.
extend could and should have a deep merge. It didn't hurt jQuery, won't hurt the underscore.

@frother
Copy link

frother commented Apr 18, 2011

I agree, this needs to be fixed. The events delegation feature is the best feature of the View class. Working around it every time I extend is crappy.

@mataspetrikas mataspetrikas reopened this Jun 8, 2011
@mataspetrikas
Copy link
Author

for some strange reason it was closed for two months :) It's an important problem and still something to be solved.

@yvg
Copy link

yvg commented Jun 8, 2011

There should clearly be a much more simpler and elegant way to merge inherited events with added events.

@gregjacobs
Copy link

Implemented this feature and made a pull request (#533) for anyone who is interested in the changes. This commit was added to version 0.5.2.

@mataspetrikas, it's not that the original (superclass) 'events' object gets overwritten, it's that it gets shadowed on the prototype chain by another Backbone.View subclass's 'events' object. Classes are not created with _.extend() (which simply merges the properties of two or more objects), they are created using prototype chains. If you're interested, google "pseudo classical inheritance", and you'll see why you can't just use a function like _.extend() or jQuery.extend() in this case :)

The implementation that I added walks up the prototype chain from a given View instance, and collects each of the 'events' objects defined along the way. At the end, these objects are merged into a single 'events' object that is used to set up the event delegation.

@jashkenas
Copy link
Owner

I'm afraid I don't agree with this change ... not only does it go against how prototype chains in JavaScript actually work, it makes it impossible for a View subclass to ignore some events that the parent View listens for.

In the current situation, it's possible to create an events hash of your own choosing ... whereas if we automatically merged in events from the parent, you'd have no way to disable them.

If you need something fancy, that's what delegateEvents(eventsConfig) is for. Just call your hash something other than events, and call that function once you've set it up to your liking.

View = Backbone.View.extend({

  initialize: function() {
    this.delegateEvents(this.buildEvents());
  }

});

@gregjacobs
Copy link

@jashkenas, If you are trying to ignore some events of a superclass in a subclass, then that means you are most likely doing something wrong in your OO design, and shouldn't be extending that superclass in the first place. (Or, you have the wrong level of abstraction in your code, and your superclass is trying to do too much). However, you can always override the handler methods in a subclass if you wanted to "ignore" certain events.

Right now there is no easy way to have the events "extend" in each subclass without fairly ugly workarounds, such as completely duplicating the events (very bad), or having something like this (in which it has to be remembered by all developers to use):

var SubView = View.extend( {
  events: _.extend( {}, View.prototype.events, {
    'subclassEvent1' : 'method1',
    'subclassEvent2' : 'method2'
  } )
} );

Or, like you mentioned with calling delegateEvents() manually, and implementing your own scheme using another variable name (but again, that has to be remembered by developers who may be looking at the documentation and thinking that it works another way).

However, the one thing I could see to be a problem is that it might be too auto-magical for people to easily recognize what's going on: that their events would be inherited rather than overwritten. But most people probably don't extend past Backbone.View anyway, so I'll leave you to decide :)

It may be convenient for developers if you decide not to implement this, to provided a method that could be overridden for how delegateEvents() collects the events hash to use. The method would simply return this.events as the default implementation, and could be overridden to allow for a prototype walking implementation like mine.

(Btw, this implementation doesn't "go against" how prototype chains work, it actually takes advantage of the fact that it's a walkable chain, unlike lexically scoped variables.)

All the best,
Greg

@jashkenas
Copy link
Owner

How about this -- we mirror the current pattern with Model's defaults. If events happens to be a function, we call it, instead of treating it as a hash. Then you can use a super call to merge in the parent's events, if desired.

In CoffeeScript:

class SubView extends ParentView

  events: ->
    _.extend super,
      subclassEvent1: 'method1'
      subclassEvent2: 'method2'

@jashkenas
Copy link
Owner

Fixed in 6bb43c1

@gregjacobs
Copy link

Hmm, unfortunately that still requires developers to remember to do something when subclassing. =/

Believe it or not, I actually like the _.extend() workaround better than a function for it anyway, as there would be no instantiation-time overhead to create the merged events hash. I was actually going to memoize the collectEvents() method that I created to avoid that overhead, but I figured that I'd start it out simple and first see if you merged in the changes :)

The best option I can really think of at this point is for an overridable method that retrieves the events from delegateEvents(). So for instance, how I changed the first line in delegateEvents() to call a method to retrieve its events (this.collectEvents()), Backbone could still do so. Ex:

delegateEvents : function(events) {
  if (!(events || (events = this.collectEvents()))) return;
  ...
}

// default, overridable implementation
collectEvents : function() {
  return this.events;
}

That would allow for a plugin or override to be written for anyone who wanted the events inheritance feature.

// override in generic Backbone.View subclass, 
// or in external plugin that overrides the method on the prototype
collectEvents : function() {
  ... walk prototypes and collect :) ...
  return mergedEvents;
}

However the more I think about it, I'm almost liking the fact that you need a workaround on the prototype, which explicitly says that the subclass's events are extending the superclass's events (like in the workaround I posted earlier). The only problem is, newbies won't know how to do that :) Perhaps just adding something to the documentation that says this is the workaround if you want this feature?

jcoglan pushed a commit to jcoglan/backbone that referenced this issue Aug 4, 2011
@forresto
Copy link

Since I'm doing this a lot, here is my tiny Backbone extension:

Backbone.View.prototype.addEvents = function(events) {
  this.delegateEvents( _.extend(_.clone(this.events), events) );
};

Use:

BaseResizable.View = Base.View.extend({
  initialize: function() {
    Base.View.prototype.initialize.call(this);
    this.addEvents({
      'resizestop': 'resizeStop'
    });
  },
  resizeStop: function(event, ui){...}
}

This was a big tripping point. If events is a hash, then manipulating it in a subview also changes it in the original view, and then Backbone fails hard because there is no resizeStop (for example) in the original view. My helper means I don't have to worry about making events a function.

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

No branches or pull requests

7 participants