-
Notifications
You must be signed in to change notification settings - Fork 132
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
fix(__extends): Use correct behaviour with null
prototype
#85
base: main
Are you sure you want to change the base?
Conversation
Does this change relate to a change in Typescript? Or should there be a change there? |
There should also be a change in TypeScript. |
tslib.js
Outdated
if (b === null) { | ||
d.prototype = Object.create(b); | ||
return; | ||
} | ||
extendStatics(d, b); |
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.
Why not just do this instead?
extendStatics(d, b); | |
if (b !== null) extendStatics(d, b); |
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.
Because it’s necessary to also add the d.prototype.constructor
property in the b === null
case.
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.
Your proposed change at the time I wrote that comment also did not set the constructor
when b === null
. Given your point about constructor
, I'd probably recommend something simpler (and with fewer characters overall):
__extends = function (d, b) {
if (typeof b !== "function" && b !== null)
throw new TypeError("Class extends value " + String(b) + " is not a constructor or null");
if (b) extendStatics(d, b);
function __() {}
(d.prototype = b ? (__.prototype = b.prototype, new __()) : Object.create(b)).constructor = d;
};
A few notes:
- I'm using
if (b)
rather thanif (b !== null)
since the null check happens on the first line and a function's "truthiness" can't be overridden throughvalueOf
,Symbol.toPrimitive
, or aProxy
- I swapped the condition on the last line from
b === null
tob
, and therefore swapped the order of expressions in the conditional. - I moved the
constructor
assignment out of the__
prototype helper and onto the last line.
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.
It might be better to fold the extendStatics
call inside the b ?
conditional to lower the amount of ToBoolean(b)
checks necessary:
function __extends(d, b) {
// This should also check that `b.prototype` is an object:
if (typeof b !== "function" && b !== null)
throw new TypeError("Class extends value " + String(b) + " is not a constructor or null");
function __() {}
(d.prototype = b
? (extendStatics(d, b), __.prototype = b.prototype, new __())
: Object.create(b)
).constructor = d;
}
f073ee5
to
b04b8d0
Compare
Co-authored-by: Ron Buckton <[email protected]>
b04b8d0
to
5f74ae1
Compare
Apparently "correct" behavior when extending Until this has been resolved in the specification, I'd rather not make any more changes to |
If you were to call
__extends(Foo, null)
, it would cause incorrect behaviour forextendStatics
, asFoo
is supposed to inherit from%Function.prototype%
in that case, instead of also inheriting fromnull
.