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

feat(lib/es2015): Add typed overloads to Reflect #35608

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Dec 10, 2019

This adds typed overloads to Reflect, similar to what strictBindCallApply does.

I’ve also corrected the type of getPrototypeOf(…) and setPrototypeOf(…). (extracted into #41987)

Depends on:

* @param argumentsList An array of argument values to be passed to the function.
*/
function apply<T, A extends any[], R>(target: (this: T, ...args: A) => R, thisArgument: T, argumentsList: A): R;
function apply(target: (...args: any) => any, thisArgument: any, argumentsList: ArrayLike<any>): any;

Choose a reason for hiding this comment

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

This second overload will just be picked by the compiler whenever anything fails to be accepted by the first overload, so this doesn't really add strictness. Better autocomplete, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this is needed to be able to pass the arguments object.

* @param newTarget The constructor to be used as the `new.target` object.
*/
function construct<A extends any[], R>(target: new (...args: A) => R, argumentsList: A, newTarget?: new (...args: any) => any): R;
function construct(target: new (...args: any) => any, argumentsList: ArrayLike<any>, newTarget?: new (...args: any) => any): any;

Choose a reason for hiding this comment

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

Same here, the second overload makes the first one useless 😢

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 3, 2020
@sandersn sandersn self-assigned this Mar 10, 2020
@typescript-bot typescript-bot added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug and removed For Backlog Bug PRs that fix a backlog bug labels Aug 4, 2020
@ExE-Boss ExE-Boss force-pushed the lib/es2015/typed-reflect branch from 6901942 to ea136c0 Compare November 6, 2020 12:22
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@ExE-Boss ExE-Boss marked this pull request as draft December 16, 2020 03:02
@ExE-Boss ExE-Boss force-pushed the lib/es2015/typed-reflect branch from ea136c0 to 902a77d Compare January 4, 2021 10:52
@ExE-Boss ExE-Boss marked this pull request as ready for review January 4, 2021 10:52
@sandersn sandersn requested review from andrewbranch and rbuckton May 10, 2022 00:11
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable improvement that wouldn't break anything obvious because it leaves the originals behind as overloads. I did a quick sample of usage and Reflect is used sparingly across many prominent packages, so I think we'd notice problems after merging if there were going to be any.

However, I'd like to get the opinion of one other team member before merging, since lib changes nearly always have hidden gotches. @andrewbranch or @rbuckton can you give your opinion?

@sandersn sandersn merged commit a123fc5 into microsoft:main Aug 9, 2022
@ExE-Boss ExE-Boss deleted the lib/es2015/typed-reflect branch August 9, 2022 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants