GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — change class inheritance code by vvakame · Pull Request #12488 · microsoft/TypeScript

Conversation

@vvakame

@vvakame Masahiro Wakame (vvakame) commented Nov 24, 2016 •
edited by DanielRosenwasser
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rhysd, Hotell, and xtuc reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Nov 24, 2016
HerringtonDarkholme
Herrington Darkholme (HerringtonDarkholme) reviewed Nov 25, 2016
View reviewed changes
Comment thread src/compiler/transformers/es2015.ts Outdated
text: `
var __extends = (this && this.__extends) || function (d, b) {
for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
Object.setPrototypeOf(d, b);

@HerringtonDarkholme Herrington Darkholme (HerringtonDarkholme) Nov 25, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vvakame Masahiro Wakame (vvakame) Nov 25, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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;
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@HerringtonDarkholme Herrington Darkholme (HerringtonDarkholme) Nov 25, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

__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.😞

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@HerringtonDarkholme Herrington Darkholme (HerringtonDarkholme) Dec 2, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Related
#9925 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vvakame Masahiro Wakame (vvakame) mentioned this pull request Nov 27, 2016
JSX: a way to make all the properties of value-based elements optional #7004
Open
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Dec 2, 2016
Design Meeting Notes, 11/30/2016 #12622
Closed
@mhegazy

Mohamed Hegazy (mhegazy) commented Dec 2, 2016

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@falsandtru

falsandtru (falsandtru) commented Dec 3, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy

Mohamed Hegazy (mhegazy) commented Dec 3, 2016

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy

Mohamed Hegazy (mhegazy) commented Dec 3, 2016

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Masahiro Wakame (vvakame) added 3 commits December 3, 2016 11:17
@vvakame
change class inheritance code
9e6a83d
@vvakame
address feedback
7ea3734
@vvakame
address microsoft#12622
570575a
@vvakame
Masahiro Wakame (vvakame) force-pushed the modify-extends branch from f968f1b to 570575a Compare December 3, 2016 02:31
@vvakame

Masahiro Wakame (vvakame) commented Dec 3, 2016

Copy link
Copy Markdown
Contributor Author

I updated my PR to fit #12622 proposal

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested changes Dec 3, 2016
View reviewed changes
Comment thread src/compiler/transformers/es2015.ts Outdated
var __extendStatics = (this && this.__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]; };

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 3, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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 Mohamed Hegazy (@mhegazy) & Ron Buckton (@rbuckton)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mhegazy Mohamed Hegazy (mhegazy) Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@rbuckton Ron Buckton (rbuckton) Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@rbuckton Ron Buckton (rbuckton) Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Masahiro Wakame (vvakame) added 3 commits December 8, 2016 22:55
@vvakame
Merge branch 'master' into modify-extends
5e57507
@vvakame
catch up
074f2b7
@vvakame
address review comment
c09a8c9
@vvakame

Masahiro Wakame (vvakame) commented Dec 8, 2016

Copy link
Copy Markdown
Contributor Author

Daniel Rosenwasser (@DanielRosenwasser) Mohamed Hegazy (@mhegazy) Ron Buckton (@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 __());
  };
})();
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Dec 21, 2016
Override helpers microsoft/tslib#14
Open
@Jessidhia

Jessica Franco (Jessidhia) commented Dec 21, 2016

Copy link
Copy Markdown

Masahiro Wakame (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Jessidhia

Jessica Franco (Jessidhia) commented Dec 21, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jods4 jods (jods4) mentioned this pull request Dec 21, 2016
Inheritance (again) aurelia/templating#463
Closed
@vvakame

Masahiro Wakame (vvakame) commented Dec 22, 2016

Copy link
Copy Markdown
Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Jessidhia

Jessica Franco (Jessidhia) commented Dec 22, 2016

Copy link
Copy Markdown

Masahiro Wakame (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Dec 26, 2016
Implementation of inheritance not compatible with Object.getOwnPropertyNames() #13163
Closed
Expected behaviour of __extends with static accessors? #1520
Closed
@mhegazy

Mohamed Hegazy (mhegazy) commented Dec 29, 2016

Copy link
Copy Markdown
Contributor

Daniel Rosenwasser (@DanielRosenwasser) and Ron Buckton (@rbuckton) what do you think?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) approved these changes Dec 30, 2016
View reviewed changes
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed Dec 30, 2016
View reviewed changes
Comment thread src/compiler/transformers/es2015.ts Outdated
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};`
var __extends = (this && this.__extends) || (function () {
var __extendStatics = Object.setPrototypeOf ||

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 30, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Dec 30, 2016
Update extends to use setPrototypeOf when available microsoft/tslib#20
Merged
@mhegazy

Mohamed Hegazy (mhegazy) commented Dec 30, 2016

Copy link
Copy Markdown
Contributor

Masahiro Wakame (@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Masahiro Wakame (vvakame) added 3 commits December 31, 2016 22:26
@vvakame
address feedback
a89bb82
@vvakame
Merge branch 'master' into modify-extends
c691269
@vvakame
update baselines
4affef2
@vvakame

Masahiro Wakame (vvakame) commented Dec 31, 2016

Copy link
Copy Markdown
Contributor Author

updated!

Jessidhia and Hotell reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy
Mohamed Hegazy (mhegazy) merged commit 700d724 into microsoft:master Jan 3, 2017
@dpogue

Darryl Pogue (dpogue) commented Jan 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy

Mohamed Hegazy (mhegazy) commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

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

For static properties with symbols as keys, yes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vvakame
Masahiro Wakame (vvakame) deleted the modify-extends branch January 4, 2017 00:01
@Jessidhia Jessica Franco (Jessidhia) mentioned this pull request Jan 4, 2017
Inheritance problem with ES6 class transpiled with BabelJS #12059
Closed
@vvakame Masahiro Wakame (vvakame) mentioned this pull request Jan 4, 2017
4.1 doesn't work with Typescript skatejs/skatejs#936
Closed
@yGuy

Sebastian Müller (yGuy) commented Jan 4, 2017

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@HerringtonDarkholme

Herrington Darkholme (HerringtonDarkholme) commented Jan 4, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@stevehansen

Steve Hansen (stevehansen) commented Jan 4, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy

Mohamed Hegazy (mhegazy) commented Jan 4, 2017

Copy link
Copy Markdown
Contributor

Steve Hansen (@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@stevehansen

Steve Hansen (stevehansen) commented Jan 4, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Mohamed Hegazy (@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);
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jods4

jods (jods4) commented Jan 6, 2017

Copy link
Copy Markdown

Sebastian Müller (@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.

DanielRosenwasser, yGuy, felixbillon, and atsu85 reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jods4

jods (jods4) commented Jan 6, 2017

Copy link
Copy Markdown

Sebastian Müller (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@yGuy

Sebastian Müller (yGuy) commented Jan 9, 2017

Copy link
Copy Markdown

jods (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jods4

jods (jods4) commented Jan 9, 2017

Copy link
Copy Markdown

Sebastian Müller (@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.

atsu85 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Jan 11, 2017
Incomplete Metadata for subclass with default constructor #13410
Closed
@aluanhaddad Aluan Haddad (aluanhaddad) mentioned this pull request Jan 14, 2017
Cannot to extends classes with static methods after Babel #13480
Closed
@jods4 jods (jods4) mentioned this pull request Mar 31, 2017
Inherited bindable properties in custom elements aurelia/templating#507
Merged
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser approved these changes
+3 more reviewers
@HerringtonDarkholme Herrington Darkholme (HerringtonDarkholme) HerringtonDarkholme left review comments
@rbuckton Ron Buckton (rbuckton) rbuckton left review comments
@mhegazy Mohamed Hegazy (mhegazy) mhegazy left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@vvakame @mhegazy @falsandtru @Jessidhia @dpogue @yGuy @HerringtonDarkholme @stevehansen @jods4 @DanielRosenwasser @rbuckton @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.