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

fix(__extends): Use correct behaviour with null prototype #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ExE-Boss
Copy link
Contributor

If you were to call __extends(Foo, null), it would cause incorrect behaviour for extendStatics, as Foo is supposed to inherit from %Function.prototype% in that case, instead of also inheriting from null.

@HolgerJeromin
Copy link

Does this change relate to a change in Typescript? Or should there be a change there?

@ExE-Boss
Copy link
Contributor Author

There should also be a change in TypeScript.

tslib.js Outdated
if (b === null) {
d.prototype = Object.create(b);
return;
}
extendStatics(d, b);
Copy link
Member

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?

Suggested change
extendStatics(d, b);
if (b !== null) extendStatics(d, b);

Copy link
Contributor Author

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.

Copy link
Member

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 than if (b !== null) since the null check happens on the first line and a function's "truthiness" can't be overridden through valueOf, Symbol.toPrimitive, or a Proxy
  • I swapped the condition on the last line from b === null to b, 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.

Copy link
Contributor Author

@ExE-Boss ExE-Boss Jan 7, 2021

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

@rbuckton
Copy link
Member

rbuckton commented Feb 8, 2022

Apparently "correct" behavior when extending null is up for debate. The algorithms in the ECMA-262 specification related to extends null actually prevent it from working, and no engine currently supports extends null. The last time the behavior was discussed in plenary was last October and the conclusion was that more time is needed to explore solutions.

Until this has been resolved in the specification, I'd rather not make any more changes to extends null semantics in TypeScript. Until then, I plan to leave this PR open so that we can come back to it once the specification issues have been resolved.

@rbuckton rbuckton added the Waiting for TC39 Unactionable until TC39 reaches some conclusion label Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for TC39 Unactionable until TC39 reaches some conclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants