Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Return types on Promise #806

Open
spadgos opened this issue Dec 5, 2018 · 3 comments
Open

Return types on Promise #806

spadgos opened this issue Dec 5, 2018 · 3 comments

Comments

@spadgos
Copy link

spadgos commented Dec 5, 2018

Some of the methods on Promise have any as their return type. Notably, Promise.prototype.then() and Promise.resolve().

For example:

static resolve < T >(value: ಠ_ಠ.clutz.goog.Promise < T , any > | T): any;

I'd have expected this to be

static resolve<T>(value: ಠ_ಠ.clutz.goog.Promise<T, any>|T): ಠ_ಠ.clutz.goog.Promise<T, any>;
@teppeis
Copy link
Contributor

teppeis commented Dec 10, 2018

#689 introduced the type definition.

// TODO(lucassloan): goog.Promise has bad types that are coerced to any, so explicitly emit any
// and change to the proper type (value: googPromise< T , any > | T): googPromise<T, any>
// when the callers have been fixed.

I don't know why it has to return any and what the "when the callers have been fixed" means. (about Google internal code?)
I think goog.Promise and goog.Thenable should be fixed like:

declare namespace ಠ_ಠ.clutz.goog {
  class Promise < TYPE , RESOLVER_CONTEXT = void > implements ಠ_ಠ.clutz.goog.Thenable < TYPE > {
    constructor (resolver : (this : RESOLVER_CONTEXT , a : (a ? : TYPE | PromiseLike < TYPE > | null | { then : any } ) => any , b : (a ? : any ) => any ) => void , opt_context ? : RESOLVER_CONTEXT ) ;
    then < RESULT > (opt_onFulfilled ? : ( (a : TYPE ) => PromiseLike < RESULT > | RESULT ) | null , opt_onRejected ? : ( (a : any ) => any ) | null) : ಠ_ಠ.clutz.goog.Promise < RESULT > ;
    static resolve < T > (value : PromiseLike < T > | T ): ಠ_ಠ.clutz.goog.Promise < T > ;
  }
}

sample code in playground

@rkirov @LucasSloan How about this? Can I send PR to fix it?

@teppeis
Copy link
Contributor

teppeis commented Jan 9, 2019

The externs for IThenable::then, Promise::catch and Promise::then in Closure Compiler were changed, adding REJECTED_VALUE to the template.
google/closure-compiler@b3df223

@rkirov
Copy link
Contributor

rkirov commented Jan 16, 2019

@teppeis Thanks for looking into this. Yes, the comment refers to google internal code, which is the primary consumer of clutz generated .d.ts files. Having 'any's was a temporary solution as we were working through getting --partial working and indeed there are many possible improvements.

Unfortunately, each improvement has to be weighted against how much current violations we have in our giant codebase and how much work it is to roll it out.

Always feel free to send me a PR and I will do that evaluation and let you know. Sometimes the answer would be that it would be prohibitively hard. I just did that for #830 and I can tell you that it is fine. Likely changing the return from ':any' to a proper type would not be, but again if you send me a PR I would follow up with describing common failure patterns. Often people forget that in real world codebases, users can mix up to 4 types of promises:

  • TS Promises
  • JS promises (translated through clutz)
  • AngularJS promises described by angularjs.d.ts
  • AngularJS promises (described by externs translated through clutz).

They are mostly runtime compatible (at least uses expect them to), so any change that makes them type-checking incompatible runs afoul of lots of existing code.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants