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

Add variadic tuple overload to Promise.all #39796

Closed
wants to merge 4 commits into from
Closed

Conversation

ahejlsberg
Copy link
Member

Experiment for now. Similar to #39336, but with a different overload definition.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 29, 2020
@ahejlsberg
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 00fa5b8. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 00fa5b8. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 00fa5b8. You can monitor the build here.

@ahejlsberg
Copy link
Member Author

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 00fa5b8. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@ahejlsberg
Copy link
Member Author

@rbuckton Interesting that this doesn't seem to break VSCode. In fact, best I can tell, none of the differences in the user tests appear to be caused by adding the extra overload.

@ahejlsberg
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 4f5b329. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 4f5b329. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 4f5b329. You can monitor the build here.

@ahejlsberg
Copy link
Member Author

Latest commit removes the old overloads. This is a breaking change for calls with explicit type arguments, but it may still be worth it for the simplicity and improvement in error reporting that it brings. Re-running tests to see the effects.

@rbuckton
Copy link
Member

I'm still very wary of Awaited<T>. The definition of Awaited<T> has some strange effects when not in strictNullChecks. @jablko also looked at this in several PRs (#35284, #33707, #33055) and made a note here: https://github.com/microsoft/TypeScript/pull/35284/files#diff-a6b488d9bd802977827b535a3011c1f3R1381

Basically, Awaited<undefined> and Awaited<null> return unknown when not in strictNullChecks, which seemed surprising.

I'd also looked into adding Awaited<T> into the core library with a definition that was intended to give a more correct answer WRT non-promise "thenables" in #21613:

type Awaited<T> =
    T extends { then(onfulfilled: (value: infer U) => any): any; } ? U :
    T extends { then(...args: any[]): any; } ? never :
    T;

This definition represented the minimal case that was consistent with await (aside from recursive unwrap), as the following is currently an error in TypeScript:

declare const p: { then(): void };
async function f() {
  await p; // Type of 'await' operand must either be a valid promise or must not contain a callable 'then' member.
}

@rbuckton
Copy link
Member

Ah, disregard the comment about undefined, as I see that it is included in your changes.

We could theoretically bake in a few levels of nesting. Its fairly trivial to result in Promise<Promise<T>> due to inference, though higher levels are much less frequent. Until such time as we can reconsider awaited, perhaps we should consider a definition like this:

type Awaited<T> =
    T extends undefined ? T :
    T extends PromiseLike<PromiseLike<infer U>> ? U :
    T extends PromiseLike<infer U> ? U :
    U;

While more verbose, it does handle both the common cases (T, Promise<T>) and the frequent doubly-nested promise case due to inference (Promise<Promise<T>>).

@@ -1,3 +1,5 @@
type Awaited<T> = T extends undefined ? T : T extends PromiseLike<infer U> ? U : T;
Copy link
Member

Choose a reason for hiding this comment

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

I would propose we move this to es5.d.ts so that it sits alongside PromiseLike, as it has no dependency on Promise directly.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Jan 25, 2022

This can probably be reevaluated now that #45350 has been merged.

@sandersn
Copy link
Member

This experiment is pretty old, so I'm going to close it to reduce the number of open PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experiment A fork with an experimental idea which might not make it into master For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants