-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Class Inheritance is Not ES6 Spec Compliant #1601
Comments
All that's really missing is var __extends = this.__extends || function (d, b) {
d.prototype.__proto__ = b.prototype;
d.__proto__ = b;
}; Notes:
|
Verified that it should indeed be (function (){"use strict"; class Foo{}; class Bar extends Foo{}; console.log(Object.getPrototypeOf(Bar) === Foo);})() prints |
|
Hmm, indeed. Only IE11 supports setting |
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 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 I would say that requiring |
@Arnavion I like this solution. I think ES3 support should be dropped, as having a enumerable |
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 |
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 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 Nevertheless just a suggestion as a meantime thing @EisenbergEffect you might use this |
@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 |
Is there currently a way to plug in a custom |
Since the default emit is If you want to do it in TS itself along with the original code, then put a |
A hacky almost working suggestion perhaps in an npm var root = global || window;
root.__extends = function(d,b){/*your code*/} Now Note: I haven't tested the code but I say almost because the default |
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 |
@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 |
@basarat 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:
var __root = typeof global !== "undefined" && global || typeof window !== "undefined" && window || this;
__root.__extends = function(d,b){/*your code*/} would work with a single 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 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 Edit: Fixed. |
BTW this is what browserify uses to shim 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 |
We'll have to discuss what exactly to do here. I know there's a lot of hesitation to make |
@RyanCavanaugh To summarize
Because of this, @basarat is proposing the default emit try to place __extends on the global object instead of 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 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: |
Can we please not have two completely different conversations here?
|
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 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 |
The following should log
true
but logsfalse
instead:This is due to how the __extends helper is implemented. It generates this:
An example of a correctly functioning extends implementation can be found in the 6to5 compiler. Here is their implementation:
The text was updated successfully, but these errors were encountered: