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

Type corrections for Promise to adhere to Promises/A+ standard #8216

Closed
wants to merge 5 commits into from

Conversation

TorstenStueber
Copy link

The type definition for promises need to be changed in two ways in order to adhere to the Promises A+ specification:

  1. The return type of then must be PromiseLike<T|U> (for PromiseLike) and Promise<T|U> (for Promise), respectively. This is due to rule 2.2.7.3 in the spec: if the argument onfulfilled of then is not defined, then the promise returned by then must be fulfilled with the same value as the originating promise – however, this value is of type T.
  2. The type of value of the argument onfulfilled of then in the interface PromiseLike must be T | PromiseLike<T> instead of T. This is partially due to rule 2.3.3.3.1 of the spec: the value y can be of type T | PromiseLike<T> instead of T only. Unfortunately, rule 2.3.3.3.1 and the entire Promises A+ standard does not explicitely state that y is also allowed to be a PromiseLike<T>. However, the Promises/A+ Compliance Test Suite will fail if y is assumed to be only of type T and not T | PromiseLike<T> (see test case 'y is a thenable' in https:/promises-aplus/promises-tests/blob/master/lib/tests/2.3.3.js).

Remark: the type of value of the argument onfulfilled of then in the class Promise must be only T, though.

For references see my repository, an implementation of the Promises A+ spec in TypeScript – in this project Promise is named Tomise and PromiseLike is referred to as Thenable.

Fixes #

Torsten Stüber added 2 commits April 20, 2016 12:39
Extend definition of the `onfulfilled` handler for the `PromiseLike` interface
Extend definiton for interface `Promise`: take into account that `onfulfilled` is undefined
@msftclas
Copy link

Hi @TorstenStueber, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

@TorstenStueber, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@TorstenStueber
Copy link
Author

TorstenStueber commented Apr 20, 2016

Obviously CI checks have failed. However, in view of the log message I cannot trace the failed tests back to my changes. I would be thankful for any help.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2016

@rbuckton can you take a look.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2016

the tests are failing and you need to accept the new baselines. jake baseline-accept

@jwbay
Copy link
Contributor

jwbay commented Apr 21, 2016

I'm just curious, what type of code will this enable/fix? From the signatures it almost looks like this is going to require continual casting or explicit annotations as you change the return type while chaining promises. For example, would this still work?

Promise.resolve(42).then(num => {
   return "string";
}).then(str => {
   return [str];
}).then(arr => {
   console.log(arr[0].toLowerCase());
});

@rbuckton
Copy link
Member

@TorstenStueber Unfortunately the typings for Promise you provided would make the more common use cases of Promise much harder to use, as @jwbay mentioned above.

The recent additions to the compiler to support non-nullable types would likely be better for your purposes:

    then<TResult>(onfulfilled: null | undefined, onrejected: (reason: any) => TResult | PromiseLike<TResult>): Promise<T | TResult>; 
    then<TResult>(onfulfilled?: (value: T) => TResult | PromiseLike<TResult>, onrejected?: (reason: any) => TResult | PromiseLike<TResult>): Promise<TResult>; 

Also be aware that the interface definition for PromiseLike is used to support async functions, and your proposed change could have an adverse effect on accurate type resolution for await.

@TorstenStueber
Copy link
Author

@rbuckton Thanks for your great comment. It just dawned to me this morning as well that one should distinguish between a null or undefined argument onfulfilled and a non-null argument onfulfilled in order to avoid the unfortunate problem that @jwbay mentioned.

Indeed, according to Promises/A+ the return value of then can only be of type Promise<T> if onfulfilled is not provided. One can even distinguish further:

  • onfulfilled non-null: result is Promise<TResult>
  • onfulfilled null and onrejected non-null: result is Promise<T|TResult>
  • onfulfilled null and onrejected null: result is Promise<T>

So out of logic necessity of the Promises/A+ spec and in order to avoid unnecessary type castings I would like to go ahead and change the PR accordingly.

About PromiseLike: this is unfortunate as well. The most correct solution would be to define yet another interface Thenable (that is similar to PromiseLike but contains my change) to be used in the Promise declaration. However, I do not want to complicate matters as this minor correction would only be required for Promise polyfill developers. Hence, I will remove this change from the PR.

Torsten Stüber added 2 commits April 21, 2016 09:03
Revert changes in PromiseLike interface
Corrections to return type of `then` method in `Promise` interface.
@rbuckton
Copy link
Member

Keep in mind the Promise/A+ term "thenable" does not mean it's a Promise or "Promise-like", it just means that it is a foreign object with a callable "then" member. { then(){} } is a thenable, but certainly not a Promise.

@Arnavion
Copy link
Contributor

Just FYI - Promise in es2015.promise.d.ts is the ES2015 Promise, not the Promises/A+ Promise, so the spec to reference is http://www.ecma-international.org/ecma-262/6.0/#sec-promise-objects . They have some implementation differences.

@TorstenStueber
Copy link
Author

@Arnavion You are right, I should have referred to the ES2015 Promise spec instead of Promises/A+. However, my changes are also required by ES2015 Promises:

  1. Section 25.4.5.3.1 of the spec defines the then method of promises: item 3 replaces onFulfilled by "Identity" if not provided. The latter is just the identity function; hence, the Promise returned by then needs to accept values of type T for its [[Resolve]] function.
  2. Section 25.4.1.3.2 specifies that resolution is allowed to have a then method and will recursively invoke PromiseResolveThenableJob for further resolution in this case.

then<TResult>(onfulfilled?: (value: T) => TResult | PromiseLike<TResult>, onrejected?: (reason: any) => TResult | PromiseLike<TResult>): Promise<TResult>;
then<TResult>(onfulfilled?: (value: T) => TResult | PromiseLike<TResult>, onrejected?: (reason: any) => void): Promise<TResult>;
then<TResult>(onfulfilled?: null | undefined, onrejected?: null | undefined): Promise<T>;
then<TResult>(onfulfilled?: null | undefined, onrejected: (reason: any) => TResult | PromiseLike<TResult>): Promise<T|TResult>;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be not optional, do not think this would compile this way anyways.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 13, 2016

With the recent updates to the library including #10448, i do not think this is needed. closing.

@mhegazy mhegazy closed this Sep 13, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants