Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ES5: inheritance of static properties is incorrect #4266

Closed
jods4 opened this issue Aug 10, 2015 · 4 comments
Closed

ES5: inheritance of static properties is incorrect #4266

jods4 opened this issue Aug 10, 2015 · 4 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@jods4
Copy link

jods4 commented Aug 10, 2015

Assume

class B {}
class C extends B {}

The default __extends is copying each own property of B to C. This is a problem because it means that a property of B becomes indistinguishable from a property of C (by virtue of copy, it is its own).

This doesn't seem like much, but sometimes you need each class to have a different value for a property.

For a real life use-case, I was bitten here: aurelia/metadata#22.
Other use-cases that occured to me because of this bug: WeakMap polyfills stores data in the object under a random key. Because of the codegen above, WeakMap doesn't work properly for B and C.
By virtue of that, Reflect.metadata polyfill is not working in IE9 and 10 (it relies on WeakMap).

Contrast with Babel, who just chains prototype. This way, one can check hasOwnProperty().
An artificial example showing the different behavior between TS and Babel:

http://www.typescriptlang.org/Playground#src=class%20B%20%7B%7D%0AB%5B%22__metadata__%22%5D%20%3D%204%3B%0Aclass%20C%20extends%20B%20%7B%7D%0Aif%20(C.hasOwnProperty(%22__metadata__%22))%0A%09alert(C%5B%22__metadata__%22%5D)%3B

http://babeljs.io/repl/#?experimental=true&evaluate=true&loose=false&spec=false&code=class%20B%20%7B%7D%0AB.__metadata__%20%3D%204%3B%0Aclass%20C%20extends%20B%20%7B%7D%0Aif%20(C.hasOwnProperty(%22__metadata__%22))%0A%20%20alert(C.__metadata__)%3B

@weswigham
Copy link
Member

I think the difference has to do with our downlevel emit not relying on either the ES5.1 Object.setPrototypeOf or the (only standardized in ES6) __proto__ - the downlevel does ruin some expected prototype chain walking semantics, but doesn't rely on either of those. See #1601 for some past discussion on the issue.

Conveniently, you can define your own global __extends function which provides the behavior you need.

@jods4
Copy link
Author

jods4 commented Aug 11, 2015

Yes, this is basically the same as #1601.
The reported issue is not the same but the underlying discussion is exactly the same.

I kinda feel bad about it because with the "modern" web stack it can easily screw you when you start using advanced features, such as metadata. I stumbled upon it by accident and it took me some time to figure it all out.

I am relying on Babel for the ES5 generation and your comment makes me realize that it won't work in IE < 11. In fact Babel is a no-op if there is no support for setPrototypeOf and __proto__. (Thankfully I don't care about inheriting statics, that's a strange idea anyway, I think).

Babel at least works for modern browsers (IE11), TS doesn't. Neither work exactly as it should in older browser, it's arguable which behavior is best...

I understand the argument in #1601 that you don't want to take a dependency "modern" features for the codegen, although I don't agree. Given that other codegen is correct in some browsers rather than always wrong; and given that people can choose to opt in less correct codegen by providing their own __extends if they need to, I'd say go with the less wrong.

Anyway, whatever the decision is I think this issue needs more visibility. Maybe you should provide alternate __extends in the documentation or even as a compiler switch. As I said the default codegen can bite you in surprising ways. The upcoming metadata features will make it a lot more visible.

@RyanCavanaugh
Copy link
Member

I think the general observation is that it's better to be slightly incorrect, but consistent, than to be correct on some platforms and incorrect on others. Browser compat testing is already a nightmare and we don't want to make it worse by having silently different behavior on different browsers.

The fact that you became aware of this now, rather than way later when you had your code running on IE10, is a very good thing.

@jods4
Copy link
Author

jods4 commented Aug 11, 2015

I see your point, one way or another you can't be correct in all situations. Being consistent on all platform is a good thing, indeed.

I still think that the metadata apis are going to make this issue very visible, yet I am unsure what you should do about it. Maybe limit attributes usage to ES6+ codegen? Along with some documentation of why this is forbidden (or behind a flag) that would make people aware of the pitfall?

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Aug 17, 2015
@mhegazy mhegazy closed this as completed Sep 17, 2015
@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.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

4 participants