Skip to content
This repository has been archived by the owner on Jun 8, 2019. It is now read-only.

Class inheritance is overkill #3

Closed
ngauthier opened this issue Nov 30, 2011 · 5 comments
Closed

Class inheritance is overkill #3

ngauthier opened this issue Nov 30, 2011 · 5 comments

Comments

@ngauthier
Copy link

in here: https://github.com/thoughtbot/backbone-support/blob/master/lib/assets/javascripts/backbone-support/swapping_router.js

You are using _'s extend which requires you to extend the prototype as well, but Backbone JS has its own extend method:

http://documentcloud.github.com/backbone/#Model-extend

It is self-propagating so objects that extend a method with this extend method get their own extend method as well:
https://github.com/documentcloud/backbone/blob/master/backbone.js#L992

And also inherits the prototype for you:
https://github.com/documentcloud/backbone/blob/master/backbone.js#L1081

When I started working with backbone, it didn't work like this, so I ended up doing something similar to what you guys have, but since they improved extend it's much easier to subclass.

Unless of course there's something I'm missing here about the behavior of extend :-)

Thanks for sharing your code.

-@ngauthier

@jferris
Copy link
Member

jferris commented Nov 30, 2011

We originally wrote this before Backbone's extend changed. I have no reason to believe this can't be updated now. Thanks for catching that.

@ngauthier
Copy link
Author

yep NP. In fact ... recipeswithbackbone/recipeswithbackbone.github.com#23 :-D

domchristie added a commit to domchristie/backbone-support that referenced this issue Sep 1, 2012
domchristie added a commit to domchristie/backbone-support that referenced this issue Sep 10, 2012
@jferris
Copy link
Member

jferris commented Nov 9, 2012

I tried using a fix similar to the change @domchristie used, but it means that you need to manually apply the CompositeView initialize method if you have your own initializer, which breaks compatibility and is a step backwards. Has anybody found a way to use Backbone's extend without manually calling initializers?

@domchristie
Copy link
Contributor

I think the inheritance pattern used in CompositeView is acceptable, given the need to add (pre-)initialize code. However, there is slight improvement that could be made.

I came across a similar issue when trying to extend the Backbone.Model class with some code that ran before the initialize method was called. Using the 'old' inheritance pattern (i.e. similar to that currently being used in CompositeView) caused all kinds of issues with Backbone collections, because Backbone.Model's prototype property was not included in the prototype chain of the new model class. Or to demonstrate with some code:

// Backbone.MyNewModel inherits from Backbone.Model using 'old' pattern

new Backbone.MyNewModel() instanceof Backbone.MyNewModel // returns true
new Backbone.MyNewModel() instanceof Backbone.Model // returns false

Relating this to CompositeView and you get:

new Support.CompositeView() instanceof Backbone.View // returns false

To fix this, I used some code from the Backbone source to 'do' inheritance. It's very similar to currently exists, but just adds the parent's prototype to the child's prototype chain:

;(function() {
  var ctor = function(){},
      View = Backbone.View,
      CompositeView;

  CompositeView = Support.CompositeView = function(options) {
    View.apply(this, [options]);
    // pre-initialize methods go here
  };

  ctor.prototype = View.prototype;
  CompositeView.prototype = new ctor();

  _.extend(CompositeView.prototype, View.prototype, {
    // instance methods go here
  });

  CompositeView.extend = View.extend;
})();

And with this pattern:

new Support.CompositeView() instanceof Support.CompositeView; // returns true
new Support.CompositeView() instanceof Backbone.View; // returns true

Now this may not be as important as with a Backbone.Model but thought it was worth sharing anyway.

@jferris
Copy link
Member

jferris commented Nov 30, 2012

@domchristie interesting stuff. I haven't run into a situation where that's mattered before, but I'd take a pull request to correct the constructor chain.

@jferris jferris closed this as completed Nov 30, 2012
@jferris jferris mentioned this issue Jul 11, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants