Skip to content

Conversation

@jablko
Copy link
Contributor

@jablkojablko commented Nov 12, 2019

Roll up #33062, #33074, #33103, #33105, #33707 and #35284

Fixes #27711
Fixes#22469
Fixes#28427
Fixes #30390
Fixes#31722
Fixes#33559
Fixes#33562
Fixes#33752
Fixes #34924
Fixes#34937
Fixes #35136
Fixes #35258

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

@jablkojablkoforce-pushed the patch-22 branch 3 times, most recently from 4855cc2 to f8e58ecCompareNovember 18, 2019 14:18
@orta
Copy link
Contributor

orta commented Nov 18, 2019

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 18, 2019

Heya @orta, I've started to run the parallelized community code test suite on this PR at f8e58ec. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 18, 2019

Heya @orta, I've started to run the extended test suite on this PR at f8e58ec. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 18, 2019

Heya @orta, I've started to run the parallelized Definitely Typed test suite on this PR at f8e58ec. You can monitor the build here. It should now contribute to this PR's status checks.

@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.

@jablkojablkoforce-pushed the patch-22 branch 5 times, most recently from 1c8c527 to 7f71fcdCompareNovember 26, 2019 15:29
@jablkojablkoforce-pushed the patch-22 branch 3 times, most recently from ad41b21 to 62a1d57CompareNovember 29, 2019 22:25
@orta
Copy link
Contributor

orta commented Jan 22, 2020

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 22, 2020

Heya @orta, I've started to run the extended test suite on this PR at 3c20282. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 22, 2020

Heya @orta, I've started to run the parallelized Definitely Typed test suite on this PR at 3c20282. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 22, 2020

Heya @orta, I've started to run the parallelized community code test suite on this PR at 3c20282. You can monitor the build here. It should now contribute to this PR's status checks.

@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.

@orta
Copy link
Contributor

orta commented Jan 23, 2020

Looks like most of that diff is just file number changing

There is a few errors though at this location - which I think could be correct. Does that feel right?

@orta
Copy link
Contributor

orta commented Jan 23, 2020

Relates to #35998

Copy link
Contributor

@ExE-BossExE-Boss left a comment

Choose a reason for hiding this comment

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

The then method only needs to be assignable to Function.

// The undefined case is for strictNullChecks false, in which case
// undefined extends PromiseLike<infer U> is true, which would otherwise
// make Awaited<undefined> -> unknown.
typeAwaited<T>=Textendsundefined ? T : TextendsPromiseLike<infer U> ? U : Textends{then(...args: any[]): any} ? unknown : T;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
typeAwaited<T>=Textendsundefined ? T : TextendsPromiseLike<infer U> ? U : Textends{then(...args: any[]): any} ? unknown : T;
typeAwaited<T>=Textendsundefined ? T : TextendsPromiseLike<infer U> ? U : Textends{then: Function} ? unknown : T;

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

👍 Works, thanks for your help! Done. ✔️

@jablkojablkoforce-pushed the patch-22 branch 2 times, most recently from 3e6abf1 to b48a356CompareJanuary 25, 2020 20:23
@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 25, 2020

Heya @orta, I've started to run the extended test suite on this PR at b48a356. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 25, 2020

Heya @orta, I've started to run the parallelized community code test suite on this PR at b48a356. You can monitor the build here. It should now contribute to this PR's status checks.

@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.

@orta
Copy link
Contributor

orta commented Jan 27, 2020

These baselines look alright to me - jablko#7

Copy link
Contributor

@rbucktonrbuckton left a comment

Choose a reason for hiding this comment

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

I do not believe this is the correct solution to this problem.

return typeAsAwaitable.awaitedTypeOfType = type;
}

const promisedType = getPromisedTypeOfPromise(type);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree with this change. This completely removes support for recursively unwrapping a non-native promise, which is a core capability of await and our type-system support for it which has existed since the feature was added.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Can a non-native promise resolve to another promise? If not, then await isn't recursive and I think this change is safe?

If it can, then I concede that implies a recursive awaited solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, non‑native promises can resolve to an object with a .then function property, which await will resolve recursively.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Wouldn't that violate Promises/A+?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jablko: non-native promise-like implementations aren't all guaranteed to be Promise/A+. In the past, JQuery's defer created something promise-like (i.e. it has a callable then that accepted fulfill/reject callbacks), but it did not recursively unwrap. This is why native await and native Promise both recursively unwrap (to defend against non-Promise/A+-compliant promise-likes)

Copy link
Contributor

Choose a reason for hiding this comment

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

awaitis recursive for non-native promises. In https://tc39.es/ecma262/#await, the value is passed to PromiseResolve which adopts the foreign promise and performs recursive unwrapping.

Copy link
ContributorAuthor

@jablkojablkoFeb 5, 2020

Choose a reason for hiding this comment

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

Thanks! I concede that await is recursive for non-compliant promises. Does supporting non-compliant promises justify introducing a new kind of type?

There's only ever been incomplete support for non-compliant promises in TypeScript. TypeScript's native promise type hasn't recursively unwrapped non-compliant promises until now. For native and A+-compliant promises, await isn't recursive because they can't be nested.

Is a non-compliant promise more likely actually an error? Supporting non-compliant promises can be error prone, masking what are otherwise errors.

@jablkojablkoforce-pushed the patch-22 branch 4 times, most recently from 7da42e8 to 034a6fcCompareFebruary 3, 2020 15:29
Copy link
Member

@sandersnsandersn left a comment

Choose a reason for hiding this comment

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

It looks like there are outstanding comments on this PR, so I'm going to request changes to help me track our PR backlog.

@jablkojablkoforce-pushed the patch-22 branch 4 times, most recently from 340aaa9 to d802893CompareFebruary 10, 2020 20:01
@jablkojablkoforce-pushed the patch-22 branch 4 times, most recently from 5cb7c3f to 49ad9c9CompareFebruary 13, 2020 15:42
You can include es2015.promise.d.ts without es2015.iterable.d.ts. It was moved to es2015.promise.d.ts in microsoft#31117
@sandersn
Copy link
Member

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.

@microsoftmicrosoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

Archived in project

7 participants

@jablko@orta@typescript-bot@sandersn@ExE-Boss@rbuckton@weswigham