Would be great if `instanceof` checks within collection were done against `collection.model` rather than `Backbone.Model` #3051

Closed
HenrikJoreteg opened this Issue Mar 7, 2014 · 20 comments

9 participants

@HenrikJoreteg

This would allow people to use their own custom models (for example: https://github.com/henrikjoreteg/human-model#human-model) with Backbone.Collections.

I can do a PR for this, just wanted to float the idea for approval first.

@jashkenas
Owner

That's a really good idea.

@jashkenas jashkenas added the change label Mar 7, 2014
@HenrikJoreteg

Want me to do a PR for this?

@caseywebdev
Collaborator

@jashkenas this won't work with the simple factory functions you were referring to in #3042

@jashkenas
Owner

Ah, there's the rub.

@caseywebdev
Collaborator

Just so everyone is on the same page...

var Collection = Backbone.Collection.extend({
  model: function (attrs) { return new Backbone.Model(attrs); }
});
var collection = new Collection({id: 1});
collection.first() instanceof collection.model; // false
@HenrikJoreteg

What if it wasn't .model but instead some other property, for example .modelConstructor that defaulted to Backbone.Model

@caseywebdev
Collaborator

That's what model is supposed to be 😉 That would solve the problem, but maybe there's a way that prevents that duplication?

@HenrikJoreteg

I agree. It's not immediately obvious to me what the benefit of supporting the factory function is.

@legastero

Sounds like there's a need for an isInstance() method instead of using instanceof directly.

If you need a factory function as .model for polymorphism, you're likely to also need to set a custom method for isInstance().

@melnikov-s

I know that _.isPlainObject is not in underscore, but wouldn't it work here?

@caseywebdev
Collaborator

It could work with factory functions if the models they produced were inherited from the factory itself. Using the example from the docs...

var Document = Backbone.Model.extend({
  constructor: function(attrs, options) {
    if (attrs.isPublic) return new PublicDocument(attrs, options);
    return new PrivateDocument(attrs, options);
  }
});
var PublicDocument = Document.extend({constructor: Backbone.Model});
var PrivateDocument = Document.extend({constructor: Backbone.Model});
var Library = Backbone.Collection.extend({model: Document});
var library = new Library([{isPublic: true}, {isPublic: false}]);
library.at(0) instanceof library.model; // true
library.at(0) instanceof PublicDocument; // true
library.at(1) instanceof library.model; // true
library.at(1) instanceof PrivateDocument; // true

But that might be a bit too fancy...

@HenrikJoreteg

I like @legastero's idea of adding this as an overridable isInstanceOf method to Collection and using that instead of hard instanceof checks.

Something like...

Collection.prototype.isInstanceOf = function (model) {
   return model instanceOf Model;
};
@HenrikJoreteg HenrikJoreteg added a commit to HenrikJoreteg/backbone that referenced this issue Mar 7, 2014
@HenrikJoreteg HenrikJoreteg Potential fix for #3051. Add isModel method to collections 7a50e8e
@HenrikJoreteg

I went ahead and just wrote it up for discussion. I'm suggesting isModel instead of isInstanceOfseemed cleaner/clearer.

@HenrikJoreteg

here it is: #3052

@jashkenas
Owner

Henrik's approach is one way to do it, at the cost of adding a new internal hook function that might never be used by anyone other than him ;)

But hard instanceof type checking is usually a bad thing. Would it be possible for us to replace the instanceof(s) with duck-type testing instead, for this case?

@asakusuma

The folks at LinkedIn (including myself) would love this new internal hook. We are currently swiveling out _prepareModel for some of our applications.

@derekbrown

+1000. This would be absolutely HUGE for the application that my team and I are building here at LinkedIn. See @asakusuma's comment for confirmation as well.

@ansorensen

Flexibility is one of the major advantages of Backbone, so being able to decouple models from collections and allowing people to swap in their own models seems like a natural win for backbone.

@zackthehuman

+1 I have actually been bitten by (the lack of) this before and it took a while to debug. Native support for this would be awesome.

@HenrikJoreteg HenrikJoreteg added a commit to HenrikJoreteg/backbone that referenced this issue Mar 24, 2014
@HenrikJoreteg HenrikJoreteg Potential fix for #3051. Add isModel method to collections 3edb085
@jashkenas
Owner

Fixed. Enjoy.

@jashkenas jashkenas added the fixed label Mar 24, 2014
@jashkenas jashkenas closed this Mar 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment