Replace desc and extend in lib/coreUtils.js with __proto__

[mjethani]

Background

There are two functions in lib/coreUtils.js that are currently no longer required: desc and extend. Both of these can be replaced with __proto__ in the object literal for the prototype.

For example, where we previously wrote this:

const {extend} = require("./coreUtils");

InvalidFilter.prototype = extend(Filter, {
  type: "invalid"
});

Now we can write this:

InvalidFilter.prototype = {
  __proto__: Filter.prototype,
  type: "invalid"
};

This is twice as fast.

What to change

Remove desc and extend from lib/coreUtils.js and replaces their use everywhere else with __proto__ in the object literal for the prototype.

[mjethani]

__proto__ in object literals appears to be ​supported on all our platforms.

[mjethani]

If we must keep the extend function, we can rewrite it as:

function extend(cls, properties)
{
  return Object.assign({__proto__: cls.prototype}, properties);
}

[kzar]

This one seems kind of at odds with #1985, do you have an opinion Sebastian?

[agiammarchi]

FWIW __proto__ is Annex B of the specifications, not a major recommendation.

However, AFAIK, if raw performance is a concern, {__proto__: value} is the fastest but obj.__proto__ = value is not recommended over setPrototypeOf (the latter grants results, the former doesn't) and same goes for obj.__proto__ vs getPrototypeOf: methods grant results, __proto__ can be a footgun (think about null objects).

[kzar]

IMO we should clarify this in our coding style guide whatever we decide, better to get the discussion out the way once.

[kzar]

Replying to agiammarchi:

...(the latter grants results, the former doesn't)...

Sorry if this is a dumb question but what do you mean by this part? I don't understand.

[agiammarchi]

Replying to kzar:

Sorry if this is a dumb question but what do you mean by this part? I don't understand.

The following summarizes why __proto__, if used as accessor, can be a foot gun.

// imagine a funny/unknown/external/native object like this one
const funny = JSON.parse('{"__proto__": null}');

// regular code that fails as accessor (not a getter)
funny.__proto__;                    // null
Object.getPrototypeOf(funny);       // Object.prototype

const proto = {another: 'object'};

// regular code that fails as accessor (not a setter)
funny.__proto__ = proto;
proto.isPrototypeOf(funny);         // false
funny.another;                      // undefined

Object.setPrototypeOf(funny, proto);
proto.isPrototypeOf(funny);         // true
funny.another;                      // "object"

As you can see, using Object public methods always deliver developers expectations (unless these are wrapped/modified, of course or the object is frozen and prototype won't be set).

However, using it inline as literal {__proto__: value} is always OK, so if that's the only pattern we'd like to use with __proto__ I agree it's both shorter to write and faster, as long as it's used in its literal form only.

I hope I've better explained my concerns.

Regards

[sergz]

Sorry if this a dumb questions but what about using of classes and removing of those desc and extend?

[agiammarchi]

Replying to sergz:

Sorry if this a dumb questions but what about using of classes and removing of those desc and extend?

AFAIK all our target browsers support classes so ... if you'd ask me, I'd say of course, use classes :-)

[sebastian]

As Dave pointed out, we deliberatly moved away from __proto__ in #1985, since it was non-standard back then. Now its standardized in ES2016 as a legacy feature which, however, doesn't make it any more appropriate to use it (in new code).

But as sergz and agiammarchi pointed out, using the new class syntax seems to be the best option. It is in compliant with the standard, not legacy, requires the least boilerplate, makes any helper functions obsolete, and should perform well.

[mjethani]

Alright, if we agree to use classes then I'll close this. Thanks!

[agiammarchi]

For record sake, and as heads up, this is a bug that took me a while to figure out:
​https://github.com/Microsoft/ChakraCore/issues/4970

MS Edge super() call might break if the extend is a legacy function that does not explicitly return a context.

The work around needed for Edge only is to transform function Legacy() {} into function Legacy() { return this; }`

Even better, I guess, if legacy classes are converted into ES6 classes as we go/need to extend them.

edit it got fixed and it will be out with the next version of Edge
​https://github.com/Microsoft/ChakraCore/pull/4670