microsoft/TypeScript

Class Inheritance is Not ES6 Spec Compliant

EisenbergEffect opened this issue · 25 comments

The following should log true but logs false instead:

class Foo {}
class Bar extends Foo {}

console.log(Object.getPrototypeOf(Bar) === Foo);

This is due to how the __extends helper is implemented. It generates this:

var __extends = this.__extends || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    __.prototype = b.prototype;
    d.prototype = new __();
};

An example of a correctly functioning extends implementation can be found in the 6to5 compiler. Here is their implementation:

var _extends = function (child, parent) {
    child.prototype = Object.create(parent.prototype, {
      constructor: {
        value: child,
        enumerable: false,
        writable: true,
        configurable: true
      }
    });
    child.__proto__ = parent;
  };

(Object.getPrototypeOf(Bar) === Foo);

All that's really missing is child.__proto__ = parent;, so I would recommend:

var __extends = this.__extends || function (d, b) {
    d.prototype.__proto__ = b.prototype;
    d.__proto__ = b;
};

Notes:

  • for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; was needed for statics. This is overcome by d.__proto__ pointing to the base.
  • d.prototype.__proto__ = b; if you are using __proto__ why not go all the (unsafe) way.
  • Object.create the team didn't use this to support ES3.

Verified that it should indeed be true by the following code run on the console of an ES6 enabled browser (e.g. chrome://flags/#enable-javascript-harmony) :

(function (){"use strict"; class Foo{}; class Bar extends Foo{}; console.log(Object.getPrototypeOf(Bar) === Foo);})() 

prints true

__proto__ was not standardized until ES6. IE, FF, V8 and JavaScriptCore atleast support it but maybe there are some ES5 browsers out there that don't?

I am not so sure about IE (note IE10):

image

Hmm, indeed. Only IE11 supports setting __proto__.

I guess we could take 6to5's shim and add TS's static-copying code to it? Then it would start giving the correct answer for Object.getPrototypeOf(Derived) on browsers which do support __proto__, and statics will continue to be copied on browsers which don't.

var __extends = function (d, b) {
    d.prototype = Object.create(b.prototype, {
        constructor: {
            value: d,
            enumerable: false,
            writable: true,
            configurable: true
        }
    });

    d.__proto__ = b;

    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
};

Setting __proto__ is still bad for performance though - getting standardized didn't change that.


I would say that requiring Object.getPrototypeOf(Derived) === Base is a relatively uncommon operation to require this change in the default emit. If user desires it, they can provide their own implementation to override the default one.

@Arnavion I like this solution. I think ES3 support should be dropped, as having a enumerable constructor on ES5 browsers with the current extends was always ugly.

Well, I was only suggesting that for the ES5 emit. TS could keep the existing emit for ES3.

But anyway, as I said I'm not a fan of mutating __proto__, so I don't support it becoming the default emit. I think the user should provide that implementation of __extends on their own if they really want that behavior (or just the 6to5 one if they don't care about Object.getPrototypeOf(Derived) on old IE).

So, the reason I brought this up is because it relates to annotations. I'm writing a library for working with AtScript metadata annotations. Annotations are stored directly on the function that they annotate. On some occasions you want to search the class hierarchy for any annotations of a certain type. To do this, you need to use Object.getPrototypeOf(Derived) recursively until you find the annotation you are looking for or hit null. If/when TypeScript adds annotations, and developers want to start working with them, there will be a greater need to handle this.

For my own case, I am only targeting Evergreen browsers, so old IE and ES3 are not a concern of mine.

to support all browsers perhaps we should use inherits (https://github.com/isaacs/inherits/blob/master/inherits_browser.js same as the core node.js util module).

Nevertheless just a suggestion as a meantime thing @EisenbergEffect you might use this inherits instead of extends and use super_ to walk up the chain.

@basarat I think @EisenbergEffect could just use the 6to5 shim he provided if he wanted to override __extends.

Also, utils.inherit() (and that browser version) doesn't inherit statics, so it's not a drop-in replacement for __extends. If you added static-copying to it, it would basically become if (Object.create) { /* 6to5 shim code */ } else { /* TS shim code */ } which would still break on old IE.

Is there currently a way to plug in a custom __extends implementation for the TypeScript compiler? I don't need it myself but I'd like to be able to tell developers using my framework how best to get the standard behavior when using TypeScript if they need it.

Since the default emit is var __extends = this.__extends || ..., if you provide a function named __extends on the global object before the TS-emitted __extends, your function will be used instead. This is a deliberate escape hatch to let users plug in their own implementation.

If you want to do it in TS itself along with the original code, then put a var __extends = function (d, b) { ... }, that is, without the this.__extends || part, to forcibly override. Example

Is there currently a way to plug in a custom __extends implementation for the TypeScript compiler

A hacky almost working suggestion perhaps in an npm es6-extend:

var root = global || window; 
root.__extends = function(d,b){/*your code*/}

Now require('es6-extend') at the root of your project just once will fix it for all files.

Note: I haven't tested the code but I say almost because the default __extends uses this.__extends and this in node isn't global, its the current module.

Perhaps I should open a bug to get the team to not have this.__extends and instead just use __extends (which will look at global on node and window on browser).

var __extends = __extends || ... will not work. It will just get undefined for the first expression even if there's already a global __extends, since its own (uninitialized) definition is in scope, so it'll always assign to __extends. Even global || window will not work if global is undeclared, since it's a TypeError to refer to undeclared variables.

You could ask for the TS emit to become

var __root = typeof global !== "undefined" && global || typeof window !== "undefined" && window || this;
var __extends = __root.__extends = __root.__extends || ...

Unfortunately the only real solution to get the global object instead of hard-coding a list of possible globals is via eval var __root = (1, eval)("this");. Maybe this isn't a big deal though.

@Arnavion how about the following:

var root = (function(){return this})(); 
root.__extends = function(d,b){/*your code*/}

Which would work if the TS emit was:

var __extends = (function () {
    return this.__extends || function (d, b) {
        for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
        function __() { this.constructor = d; }
        __.prototype = b.prototype;
        d.prototype = new __();
    }
})();

getting a consistent this (window or global) in every file in your project.

@basarat The IIFE technique for getting the global doesn't work in strict mode. The eval technique does.

The IIFE technique for getting the global doesn't work in strict mode. The eval technique does.

My bad. Thanks.

@Arnavion making your suggestion more concrete:

fix.js:

var __root = typeof global !== "undefined" && global || typeof window !== "undefined" && window || this;
__root.__extends = function(d,b){/*your code*/}

would work with a single require('fix') if ts emit for __extends was (which is backward compatible):

var __root = typeof global !== "undefined" && global || typeof window !== "undefined" && window || this;
var __extends = __root.__extends = __root.__extends || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    __.prototype = b.prototype;
    d.prototype = new __();
}

Yes.

To be honest, we could still use your require("es6-extend") idea in this way:

this.__extends = require("es6-extend");

es6-extend would export the new function instead of assigning it anywhere. Then we can leave the current emit unchanged. The downside is that the user of the module has to remember to always assign it to this.__extends and nothing else.

Edit: Fixed.

BTW this is what browserify uses to shim global. You could use that for __root.

The evolution of that shim is interesting. It started as just returning 'self' unconditionally, then added the typeof-check for self and returned window or {} otherwise, then finally updated to check for global too. Note the comment on the first commit - I'm not sure what environment has window but doesn't have self, but browserify evidently supports global on it.

We'll have to discuss what exactly to do here. I know there's a lot of hesitation to make __extends any more complex than it absolutely has to be, but this might be important enough to change it.

@RyanCavanaugh To summarize

  • this, self and window in a script loaded in a browser refers to the global object.
  • this on node can either be the global object (when a file or script is run as the main module) or a module object (when a file is require'd). However global refers to the global object in both cases.
  • Because this is a module-specific object in a require'd piece of code, it is not possible for a require'd module to override the TS-emitted __extends implementation in the module it's being require'd into. It has to be necessarily overriden in the same file that contains it, and this has to be done in every file that has the emit.

Because of this, @basarat is proposing the default emit try to place __extends on the global object instead of this. The new emit is the second code block in #1601 (comment) This new emit tries to find a few well-known globals (node and browser) and falls back to this if it can't. It checks for and attaches the __extends function to this object. This way, a module can be written to override __extends (like the first block in that comment), and any module which wishes to use it can simply require() it.

The only user-visible difference from the current emit that I can think of is that, if the user is on node and already has a different __extends registered on the global object, it will now be clobbered. However users should not expect names prefixed with two underscores to be available to them, so I think this is acceptable. Also, if they already have a variable named __root in scope it will be clobbered. The same reasoning applies there. TS could complain about __root being used by user code, like it complains about user-defined _this, _i, etc. (TS2397-2402).

Alternatively, it could be done in an IIFE that only exposes __extends:

var __extends = (function (root) {
    return root.__extends = root.__extends || function (d, b) {
        for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
        function __() { this.constructor = d; }
        __.prototype = b.prototype;
        d.prototype = new __();
    };
})(typeof global !== "undefined" && global || typeof window !== "undefined" && window || this);

Then no new variable is introduced and no new diagnostic is needed from TS.

I think this is not much more complicated than the current __extends. In particular, it should have no difference from the current emit in terms of performance, because the actual __extends implementation is still the same - it does not hold on to anything new from its closure.

Edit:
For users who are already overridding __extends with their own var __extends declaration in the same file (both browser and node), this will continue to clobber the TS-emitted function. For users who override it in browser JS in a separate <script> block (either with var or by assigning to window.__extends), this will still continue to work.

Can we please not have two completely different conversations here?

  • How __extends interacts with the global object
  • Whether __extends sets up the prototype chain the same as ES6

Please see Ron's comments in #1622 for a proposed change to emit to better work with the global object.

For the actual bug in the original report, we've decided to keep things the way they are. The reasoning is that __proto__ is an ES6 construct, as is Object.setPrototypeOf, and without either of those, we can't emulate the ES6 prototype chain in the first place.

While some runtimes support these things, we can't make meaningful calls based on vendor-specific supported subsets of of ES6 behavior. In other words, if you want the ES6 prototype chain behavior, you'll need to target ES6. If you target ES5, the functions we would need to set the correct prototypes are not available as defined by the standard, so we're not going to try to match that behavior in only some subset of runtimes.

With the changes proposed in #1622, it should be meaningfully easier to substitute in your own "ES5.5"-compliant version of __extends that does set the prototype chain in runtimes that support it.