jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.09k stars 5.39k forks source link

Error: "Cannot read property 'idAttribute' of undefined" in collection #4203

Closed avasuro closed 2 years ago

avasuro commented 6 years ago

Description

If I create new collection class, that extends Backbone.Collection, and define it's "model" property as a function which returns instantiated model - then on collection constructed error "Cannot read property 'idAttribute' of undefined" is thrown. That's because class methods has no property "prototype", which is used in Backbone.Collection.prototype.modelId method.

Expected behavior

No error thrown if collection has property "model" defined as class method.

Actual behavior

Error "Cannot read property 'idAttribute' of undefined" thrown. Here is an example: https://jsfiddle.net/e1ybjmw3/

Environment

  1. Backbone version: 1.3.3
  2. Additional build tools, etc: no build tools (that's important, because if babel used all works fine)
dreyks commented 5 years ago

that's because class properties as well as arrow functions do not have prototype hence this fails https://github.com/jashkenas/backbone/blob/27f7d41de1f64f6662a96f44531d7518953b1e07/backbone.js#L1102

var Class1 = function() {
  this.func = function(){}
}
class Class2 {
  func() {}
}
console.log((new Class1).func.prototype) // {constructor: ƒ}
console.log((new Class2).func.prototype) // undefined
dreyks commented 5 years ago

workaround: make model a plain old function

https://jsfiddle.net/mwd7fju2/1/

const modelFactory = function(attrs, options) { return new Backbone.Model(attrs, options); }

class SomeCollection extends Backbone.Collection {
}
SomeCollection.prototype.model = modelFactory

new SomeCollection([
    {
        id: 'someModelId'
    }
]);
avasuro commented 5 years ago

Yes, but it looks unpleasant, especially if this is the only situation in application where such approach (extending classes directly through prototype) is used, plus if the collection is extended with many methods then you need to scroll down several screens to see the "model" property. It kills all the benefits of syntaxic sugar of es6 classes.

dreyks commented 5 years ago

yeah that's ugly. I'm writing in coffeescript so it doesn't look that bad for me, but still a hack

syntruth commented 5 years ago

This is my patched solution:

https://gist.github.com/syntruth/a76762aab0e442833bfd88bd937aa766

...it's not pretty, but it's working for now.

If I wasn't using Backbone Stickit, I'd moved to the Backbone-ES6 version.

dreyks commented 5 years ago

Backbone-ES6 is literally the same code but written in ES6. I has all the same issues

ogonkov commented 4 years ago

Another way, to make model a function, w/out using prototype is use get, i found it a bit more good-looking

class C extends Backbone.Collection {
  get model() {
    return function(attrs, opts) {
      return new M(attrs, opts);
    };
  }
}
jgonggrijp commented 2 years ago

If you set model to a function that returns an instance (and doesn't have a prototype.idAttribute), you need to define modelId as well. Documented over here:

If your collection uses a model factory and those models have an idAttribute other than id you must override this method.