clutz icon indicating copy to clipboard operation
clutz copied to clipboard

Return types on Promise

Open spadgos opened this issue 7 years ago • 3 comments

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>;

spadgos avatar Dec 05 '18 17:12 spadgos

https://github.com/angular/clutz/pull/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 avatar Dec 10 '18 13:12 teppeis

The externs for IThenable::then, Promise::catch and Promise::then in Closure Compiler were changed, adding REJECTED_VALUE to the template. https://github.com/google/closure-compiler/commit/b3df223618d87495e749532d1dd216370bb4b7c3

teppeis avatar Jan 09 '19 01:01 teppeis

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

rkirov avatar Jan 16 '19 23:01 rkirov