Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions backbone.js
Original file line number Diff line number Diff line change
Expand Up @@ -2107,9 +2107,16 @@

// The constructor function for the new subclass is either defined by you
// (the "constructor" property in your `extend` definition), or defaulted
// by us to simply call the parent constructor.
// by us to simply call the parent constructor. ES6 method shorthand
// (`constructor() {}`) produces a non-constructable function with no
// `prototype`; in that case we wrap it so `new` still works.
if (protoProps && _.has(protoProps, 'constructor')) {
child = protoProps.constructor;
var supplied = protoProps.constructor;
if (supplied.prototype) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine making this check a bit more precise, in one of the following ways:

Suggested change
if (supplied.prototype) {
if (_.has(supplied, 'prototype')) {
Suggested change
if (supplied.prototype) {
if (typeof supplied.prototype === 'object') {

(combination of both)

Suggested change
if (supplied.prototype) {
if (_.has(supplied, 'prototype') && typeof supplied.prototype === 'object') {

Can we think of situations where the distinction might be important enough that one of the more precise versions might be preferred? I can't quite put my finger on it, but I have a feeling that it might be important that the prototype is an own property of constructor, which would warrant the _.has check.

CC @GammaGames @Yahasana

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Would be good to switch to _.has(supplied, 'prototype').

Walking the realistic inputs (classic functions, classes, shorthand, arrow/async/generator), the truthy check and _.has behave identically. Own-vs-inherited only diverges if someone does Object.setPrototypeOf on a shorthand method, which isn't a real user path. But _.has expresses the intent directly ("supplied a function with its own prototype slot") instead of leaning on truthiness as a proxy, so it's the better line at the same cost.

We could skip the typeof === 'object' variant. The only way to fail it with a truthy prototype is manual assignment like Fn.prototype = 42, which is sabotage either way.

child = supplied;
} else {
child = function(){ return supplied.apply(this, arguments); };
}
} else {
child = function(){ return parent.apply(this, arguments); };
}
Expand Down
22 changes: 22 additions & 0 deletions test/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,28 @@
assert.notEqual(model.cid, undefined);
});

QUnit.test('extend supports ES6 method-shorthand constructor (#4213)', function(assert) {
assert.expect(2);
// ES6 method shorthand (`constructor() {}`) produces a non-constructable
// function with no `prototype` property, which previously made
// `new Model()` throw "Model is not a constructor". We use `eval` here
// so this file still parses on engines without ES6 method shorthand.
var protoProps;
try {
/* eslint-disable no-eval */
protoProps = eval('({ constructor() { Backbone.Model.apply(this, arguments); } })');
/* eslint-enable no-eval */
} catch (e) {
assert.ok(true, 'ES6 method shorthand not supported in this environment, skipping');
assert.ok(true);
return;
}
var Model = Backbone.Model.extend(protoProps);
var model = new Model({foo: 'bar'});
assert.ok(model instanceof Model);
assert.equal(model.get('foo'), 'bar');
});

QUnit.test('parse can return null', function(assert) {
assert.expect(1);
var Model = Backbone.Model.extend({
Expand Down
Loading