change class inheritance code

[vvakame]

Fixes / #3610 partially

This change makes happy with babel generated code.
related skatejs/skatejs#936

In ECMAScript 2015.
class member must be enumerable: false, but tsc generated es5 code is enumerable: true.

class Test {
  static foo() {}
}
let d = Object.getOwnPropertyDescriptor(Test, "foo");
// {"writable":true,"enumerable":false,"configurable":true}
console.log(JSON.stringify(d));

TypeScript's helper function.

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

this function copies enumerable: true property only.
I replace for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; to Object.setPrototypeOf(d, b);.

I do not know whether it is complicated and understanding is correct or not...
14.5.14 Runtime Semantics: ClassDefinitionEvaluation -> 14.5.14.12. ... DefineMethod... -> 14.3.8 Runtime Semantics: DefineMethod -> 14.3.8.7. ... MakeMethod ... -> 9.2.10 MakeMethod -> 9.2.10.3 ...[[HomeObject]] ... -> 9.2 ECMAScript Function Objects If the function uses super, this is the object whose [[GetPrototypeOf]] provides the object where super property lookups begin.

[HerringtonDarkholme]

I'm not familiar with the new transformer based emitter. But it seems Object.setPrototypeOf is not supported in ES3 environment and it is emitted in ES3 target. Does it need transformed in other transformers?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf

[vvakame]

We can use d.__proto__ = b;.
but it is not official spec in ES3 and ES5.
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Object/proto

I misunderstood, Object.setPrototypeOf is a specification from ES2015. not ES5...
We should not use Object.setPrototypeOf.

Is this code correct?

if(typeof Object.setPrototypeOf === "function") {
  Object.setPrototypeOf(d, b);
} else {
  d.__proto__ = b;
}

[HerringtonDarkholme]

__proto__ can be widely used but still not IE. https://babeljs.io/docs/usage/caveats/

I'm afraid we must do special emission for ES3.😞

[HerringtonDarkholme]

Related
#9925 (comment)

[mhegazy]

@vvakame we have an alternate proposal in #12622 to avoid breaking IE8, IE9, and IE10 users.

[falsandtru]

if we do this we break the static inheritance for IE10,

Even if this change will be optional, if someone adds and exposes this incompatibility with IE to npm, it will pollute all downstream packages. Then many packages won't work with IE because of this change. I am very worried about it.

[mhegazy]

if we do this we break the static inheritance for IE10,

Even if this change will be optional, if someone adds and exposes this incompatibility with IE to npm, it will pollute all downstream packages. Then many packages won't work with IE because of this change. I am very worried about it.

that is why we are not planning on doing it.

[mhegazy]

I should clarify. this is about emitting methods and accessors as enumrable:false. we still like to change __extends to use setProrotypeof when available.

[vvakame]

I updated my PR to fit #12622 proposal

[DanielRosenwasser]

I think this is our first helper that is depended on by another helper. Additionally, if we port this to tslib, you have a strange issue which is that even if you create your own __extendStatics helper, you can't change the behavior of an already-imported __extends helper.

This is because even after you import your helper, __extendStatics will have been captured by __extends's closure within tslib.

Thoughts @mhegazy & @rbuckton?

[mhegazy]

good point. then we do not need the (this && this.__extendStatics) part?

You can always override the __extends behavior, so it is not that we are restricting access t this functionality.

[rbuckton]

If you are using --importHelpers then you can't override any imported helper in any case.

[rbuckton]

You would need the (this && this.__extendStatics) just like any other helper for when --importHelpers is not present, and this needs to be added to tslib.

[vvakame]

@DanielRosenwasser @mhegazy @rbuckton thank you for review.
I'm using this design.
But It brings new complexity and I do not think the benefits will increase so much.

I tried below code. How is it?

var __extends = (this && this.__extends) || (function () {
  var __extendStatics = Object.setPrototypeOf ||
    ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
    function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
  return function (d, b) {
    __extendStatics(d, b);
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
  };
})();

[Jessidhia]

@vvakame From a quick look at compat-table, it seems that the only browsers that actually support { __proto__: ... } also support Object.setPrototypeOf?

It might also be important to set the __proto__ property regardless of actual support, to ensure deep inheritance sees the correct supers. See babel/babel#3527.

[Jessidhia]

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

This could use d.defineProperty(p, b.getOwnPropertyDescriptor(p)) instead of the = assignment to preserve getters / setters / enumerability; but would be problematic (throw) on IE8.

EDIT: ah, preserving setters and configurability could be troublesome when trying to actually override in a subclass.

[vvakame]

@Kovensky good point.
I'm not understanding this repo is follow the specification or follow the browser implementation history.
I would like to get advice from repository members.

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

this implementations is inherit from current __extends implementations.
It is good for backward compatibility.
I want to keep the small changeset in this pull request.

[Jessidhia]

@vvakame the current code is probably safer; actually copying the descriptors could be dangerous if the original descriptor is configurable: false, though I don't know how you could generate one in TypeScript other than by Object.definePropertying yourself.

[mhegazy]

@DanielRosenwasser and @rbuckton what do you think?

[DanielRosenwasser]

Since this isn't a helper, can we just call it staticExtend, extendStatic, or extendStatics?

[mhegazy]

@vvakame can you update the name of __extendStatics? we should be good to go otherwise.

I have pushed a similar PR to tslib at: microsoft/tslib#20

[vvakame]

updated!

[dpogue]

Does this allow transpiled classes to properly inherit symbols from parent classes?

I seem to recall that previously that worked in ES6 and in Babel, but not in TypeScript (see #7529)

[mhegazy]

Does this allow transpiled classes to properly inherit symbols from parent classes?

For static properties with symbols as keys, yes.

[yGuy]

I hope that this really is an improvement and not makes the situation even worse: Could someone do a writeup as to how typescript generated code will behave differently depending on the target switch and the browser it is being executed in? As far as I can see, now code will not only behave differently if compiled to es6 and then babelized to es5, but also it will behave differently if run on those browsers that don't support setPrototypeOf and those that do (which is IE9/10 and not so old Android and Safari browsers: compat table).
As far as I can see you will still run into trouble if you are using static properties, or elements defined as writable:false in the baseclass. This may not or infrequently happen with Typescript classes, but it sure does with third party code and this is one thing that Typescript really needs to consider: It is used a lot in scenarios where there is third party/non-TS code involved. Not everything is Typescript.
If you are doing a breaking change anyway, I would wait for committing that change and first see if you can't fix the problems with the simple assignment that is still being used in the new variant and rather go for the "defineProperty" approach that uses descriptors, honors "writable" and "configurable" and actually works with properties without breaking code.

[HerringtonDarkholme]

This is a best effort change we can achieve in TypeScript. Please refer to #9925 (comment) to see why changing inheritance code in even pure TypeScript environment will be hard.

However, this change is required for ES6 compatibility. For now this change seems to make life harder to work with JavaScript code, but it will be no less difficult in future to write ES6 along with legacy ES5. So just think it as an investment in future.

TLDR;
In practice, you can just add noLib and provide your own runtime helpers that meet your need.

[stevehansen]

It appears this change just took us by surprise, can anyone verify here if this is related?

class A {
    private static test = [];

    do() {
        B.test.push("A");
    }
}

class B extends A {
}

In the online playground this works as expected (weird, but expected). But with this change it appears that B.test is undefined.

[mhegazy]

@stevehansen can you provide more details.. this seems to be working fine for me on node.

c:\test>type a.ts
class A {
    private static test = [];

    do() {
        return B.test.push("A");
    }
}

class B extends A {
}

console.log("new A().do() => " + new A().do());
console.log("new B().do() => "+ new B().do());

c:\test>tsc a.ts  --t es5

c:\test>type a.js
var __extends = ...
var A = (function () {
    function A() {
    }
    A.prototype.do = function () {
        return B.test.push("A");
    };
    return A;
}());
A.test = [];
var B = (function (_super) {
    __extends(B, _super);
    function B() {
        return _super.apply(this, arguments) || this;
    }
    return B;
}(A));
console.log("new A().do() => " + new A().do());
console.log("new B().do() => " + new B().do());

c:\test>node a.js
new A().do() => 1
new B().do() => 2

[stevehansen]

@mhegazy, I'm sorry, it seems you are right.

Going to have to figure out was is exactly wrong in our case, what we are seeing is that B.test is undefined, and when I looked at the possible differences I saw the extendStatics call inside _extends which led me here.

I was at least able to reproduce our issue but it seems that the new code from this pull request actually fixes the problem instead of causing it, I'll have to verify with the colleague tomorrow that first got the issue.

https://jsbin.com/yosamebahu/edit?js,console

function register(name: string): (obj: any) => void {
    return (obj: any) => document.registerElement(name, obj);
}

@register("x-a")
class A {
    private static test = [];

    do() {
        return B.test.push("A");
    }
}

class B extends A {
}

console.log(typeof A.test);
console.log(typeof B.test);

[jods4]

@yGuy

Could someone do a writeup as to how typescript generated code will behave differently depending on the target switch and the browser it is being executed in?

As I've been asking for this change for a long time, sure I'll explain the underlying issue and consequences.

Intro

This only touches inheritance of static members.
Shortly, ES6 specs says that static members are inherited. So the following code needs to work:

class B {
  static x = 42;
}

class D extends B { 
}

D.x; // = 42

The problem is that you can't transpile that to ES5 in a compliant way for older browsers (think IE10-).

Until now (TS < 2.2), MS preferred having a consistently incorrect ES5 emit, i.e. the same non-ES6 compliant behavior in all browsers.
With 2.2 this will change so that we get compliant behavior when it's possible.

What browsers are impacted?

When targeting ES5 or ES3
Browsers that support either Object.setPrototypeOf() or __proto__ will get the new 2.2 correct behavior.
Other legacy browsers will continue to work like with 2.1.
Basically, this means all browsers except IE10- are fixed.

What is the difference in behavior?

With 2.2+ codegen, static members are inherited through prototype chaining.
With 2.1- codegen, static members were simply copied when defining the class.
The most visible consequences are:

• Inherited (2.2) properties are not "own" properties, while copied (2.1) are.
• Changing a property on B at runtime will now (2.2) reflect on D (unless it's overriden of course).

This may look like narrow edge cases, but it can causes major headaches, e.g. when polyfilling Reflect.metadata.
With 2.2 we can finally have working code in modern browsers. If you target IE10-, unfortunately you have to not rely on such subtleties because they cannot be emulated properly.

If you have code that depends on the old behavior, this is a breaking change. In that case, note that changing the target to ES6 would also have been a breaking change for you.

[jods4]

@yGuy

not only behave differently if compiled to es6 and then babelized to es5

Actually: now (2.2) ES5 code emitted by TS will behave like Babel in modern browsers and like native ES6.

In legacy browsers, TS and babel are both wrong but in different ways. Babel doesn't give a shit about inherited static members (D.x === undefined). TS copies them, so basic code works but with subtly incorrect behavior (D.hasOwnProperty('x') == true).

it will behave differently if run on those browsers that don't support setPrototypeOf and those that do (which is IE9/10 and not so old Android and Safari browsers: compat table).

There is a second fallback, Safari 5.1 supports setting __proto__, so does Android 4.0.
So it's really about IE10-.

first see if you can't fix the problems with the simple assignment that is still being used in the new variant and rather go for the "defineProperty" approach that uses descriptors, honors "writable" and "configurable" and actually works with properties without breaking code.

defineProperty will always be an OwnProperty, which is incorrect.
There is just no way to generate ES6 behavior in browsers that don't support setting the prototype.

[yGuy]

@jods4 - thanks for the writeup.
Don't you think it would be cool if tsc could emit warnings (if not errors) when it detects that the code has these issues, i.e. when the target is set to es5 and there is an access to a static property member that is inherited?
That said, I would rather trade in the "OwnProperty" issue for the the broken static inherited getter issue. While the latter is almost certainly guaranteed to fail, having a property declared as an OwnProperty rather than via prototype inheritance is far less likely to be an issue in an application. For fields, one could even improve the situation with a synthetic getter/setter pair that delegates to the "prototype" or stores the value locally, but still, I would say a warning is probably the best solution and these issues should be avoided in general.

[jods4]

@yGuy

Don't you think it would be cool if tsc could emit warnings (if not errors) when it detects that the code has these issues, i.e. when the target is set to es5 and there is an access to a static property member that is inherited?

It's not that simple.
In my case all static property accesses occur from external JS libraries. In fact I don't even declare a problematic property, but modern JS frameworks try to attach metadata to my classes and the copy thing creates confusion.

That said, I would rather trade in the "OwnProperty" issue for the the broken static inherited getter issue.

First: own properties are a real issue that causes major trouble. See for example aurelia/metadata#22.

Second: correct me if I'm wrong but if I understand correctly the "broken static inherited getter" is that currently a static getter is not inherited, rather its value when the derived class is declared is copied, right? This is not a "trade". It's also fixed by the prototype chaining (i.e. fixed everywhere except in IE10- where the old codegen is still applied).

---

It seems to me that the new codegen is compliant with the ES6 specs. It is also pretty much the same as what Babel does. So I'm unsure what could be better in modern browsers.
The only culprit remaining is IE10-. This can be discussed but it is about what is the "less worse" as a perfect emulation won't be possible there.

As a final note, remember you can easily substitue your own __extends to suit your needs. This is what I've been doing so far as I really need an ES6 compliant inheritance.