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

Support ES6 method definitions for constructors #4213

Open sebamarynissen opened 5 years ago

sebamarynissen commented 5 years ago

Using ES6, it becomes possible to extend Backbone objects using

const MyModel = Backbone.Model.extend({
  foo() {
    return 'bar';
  }
});

This syntax is a nice shortcut for the more old-fashioned

const MyModel = Backbone.Model.extend({
  foo: function() {
    return 'bar';
  }
});

The problem is though that if you want to specify a custom constructor

const MyModel = Backbone.Model.extend({
  constructor() {
    Backbone.Model.apply(this, arguments);
  }
});

it turns out that

let model = new MyModel();

throws an Uncaught TypeError: MyModel is not a constructor.

The reason behind this behavior is that the ES6 method definition name() is not completely equivalent to the old name: function() {} because the ES6 methods are labelled internally as non-constructable.

A way to solve this is to change the extend method from

if (protoProps && _.has(protoProps, 'constructor')) {
  child = protoProps.constructor;
} else {
  child = function(){ return parent.apply(this, arguments); };
}

to

if (protoProps && _.has(protoProps, 'constructor')) {
  var ctor = protoProps.constructor;
  child = function(){ return ctor.apply(this, arguments); };
} else {
  child = function(){ return parent.apply(this, arguments); };
}

There is one minor drawback to this approach which is that if someone ever did something like

const proto = {
  constructor: function() {}
};
const Model = Backbone.Model.extend(proto);
if (Model === proto.constructor) { ... }

this would no longer work. I tried to find whether it is possible to check whether a method is constructable or not in which case something could be done like

if (protoProps && _.has(protoProps, 'constructor')) {
  var ctor = protoProps.constructor;
  if (!isConstructable(ctor)) {
    child = function(){ return ctor.apply(this, arguments); };
  } else {
    child = ctor;
  }
} else {
  child = function(){ return parent.apply(this, arguments); };
}

but it seems that there's no elegant way to check this.

Hence it's a trade-off between 100% backwards compatability for a rather rare edge case, or to be able to get rid of the requirement to specify the constructor as a non-ES6 method definition, but this leads to some awkward code like

const Model = Backbone.Model.extend({
  constructor: function() { ... },
  method() { ... }
});

I would love to hear some thoughts on this subject. I'd be willing to create a pull request if required.

taburetkin commented 5 years ago

i have to say, that babel converts constructor() to constructor: function() also, microsoft edge understands this correctly constructor() and there is no errors if you are in edge check thisout: https://codepen.io/dimatabu/pen/ZZgdxj?editors=0010

personally, i am always using babel and faced with this issue only when i create something in test purposes and i am always treat this as bad es6 implementation in browser

sebamarynissen commented 5 years ago

I know that using Babel solves the issue, but still I think that there's a place for this, because:

jgonggrijp commented 2 years ago

Related to #3560 and #4245.