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

Support ES6 method definitions for constructors #4213

Open
sebamarynissen opened this issue May 3, 2019 · 3 comments
Open

Support ES6 method definitions for constructors #4213

sebamarynissen opened this issue May 3, 2019 · 3 comments
Labels
Projects

Comments

@sebamarynissen
Copy link

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
Copy link

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
Copy link
Author

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

  • When using platforms like electron, you don't need to use babel because you know what version of node you're running and hence there's no need to transpile ES6 functionality.
  • I don't really know the ES spec in detail, but if I'm not mistaken the fact that object methods are not constructable is in the spec, which only means that Edge doesn't follow the spec appropriately.

@jgonggrijp
Copy link
Collaborator

Related to #3560 and #4245.

@jgonggrijp jgonggrijp added this to Low priority in Dusting off Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Dusting off
Low priority
Development

No branches or pull requests

3 participants