Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Should we be using Object.create() to hook up prototypes? #872

Closed
njx opened this issue May 12, 2012 · 7 comments
Closed

Should we be using Object.create() to hook up prototypes? #872

njx opened this issue May 12, 2012 · 7 comments

Comments

@njx
Copy link
Contributor

njx commented May 12, 2012

Currently, our prototype inheritance for inline editors is set up like this (with details omitted):

function MultiRangeInlineEditor() {
    InlineTextEditor.call(this);
    // ...
}
MultiRangeInlineEditor.prototype = new InlineTextEditor();

I think this means that the InlineTextEditor() constructor gets called twice--once for the prototype itself, and then once each time we construct the editor. Not a huge deal, but it seems redundant. Should we be doing MultiRangeInlineEditor.prototype = Object.create(InlineTextEditor.prototype) instead?

See the last section in http://www.bennadel.com/blog/2184-Object-create-Improves-Constructor-Based-Inheritance-In-Javascript-It-Doesn-t-Replace-It.htm

@twolfson
Copy link

Prototype chains while awesome are a bitch to work with in reality. The best idea is to steer clear of them altogether by doing something like duck punching:

var key,
     parentProto = InlineTextEditor.prototype,
     childProto = MultiRangeInlineEditor.prototype;

// Go through the properties of the parent prototype
for (key in parentProto) {
  // If the child prototype does not have the property, copy it over
  if (parentProto.hasOwnProperty(key) && !childProto.hasOwnProperty(key)) {
    childProto[key] = parentProto[key];
  }
}

Since most prototypes don't contain objects (excluding functions), this works extremely well to flatten the inheritance structure. Additionally, if you need to reference a 'super' function at some point, you grab it out of the parent proto and reference it explicitly.

// Localize the super function from the parent proto
var InlineDoSomething = InlineTextEditor.prototype.doSomething;

// Define our child function
MultiRangeInlineEditor.prototype.doSomething = function (param) {
  // Call the super function as wanted
  InlineDoSomething.call(this, param);
  // Do something else
};

@jarek-foksa
Copy link

That article has some misconceptions, e.g. there is really no need to duplicate properties in both object literal and initializer, you first override properties from prototype and then call initilizer - not the other way around.

I would strongly recommend you to read this article as a better reference: http://killdream.github.com/blog/2011/10/understanding-javascript-oop/

If you are intending to use Object.create() then you could ditch constructors altogether. With Object.create and couple of helper functions it's much easier to build prototype chains than with constructors: https://gist.github.com/1558929

@domenic
Copy link

domenic commented May 13, 2012

Everyone seems to have used this issue to bring up their personal favorite way to do inheritance in JavaScript. I'll avoid that, and say: yes, switching to Object.create is a great idea.

@pthiess
Copy link
Contributor

pthiess commented May 22, 2012

Reviewed.

@ghost ghost assigned njx Jun 21, 2012
@njx
Copy link
Contributor Author

njx commented Dec 6, 2012

Assigning to sprint 18. It turns out that using new() to hook up the prototype actually can cause subtle bugs. In particular, if a superclass ctor uses the standard this.myHandler = this.myHandler.bind(this) pattern, the handler will get bound to the dummy prototype instance (even though it gets "re-bound" when the ctor is called on the subclass instance later, the initial binding wins).

njx added a commit that referenced this issue Dec 6, 2012
…get base class.

This addresses #2221 and #2218.
As part of this, cleaned up some issues around how we're doing inheritance:
* For #872, switched to using `Object.create()` to hook up prototypes
  (this avoids subtle bugs when binding event handlers in base classes).
* Made it so all child classes call base class implementation for overridden functions
  (even if the base class impl is empty right now). The one exception is `close()`, which
  is a bit weird right now--will file a separate issue on this.
@peterflynn
Copy link
Member

@njx: do you want to do anything more here before closing?

@njx
Copy link
Contributor Author

njx commented Dec 11, 2012

Nope, I think we're good. Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants