Improve the typings for goog.Promise. by LucasSloan · Pull Request #689 · angular/clutz

LucasSloan · 2018-02-13T00:33:58Z

Clutz hard codes translations for TTE types, and goog.Promise has 2 type parameters, which wasn't previously accounted for in the translations.

Had to turn on noStrictGenericChecks to make it compile, ts doesn't like extending promise-y things.

rkirov · 2018-02-13T01:46:51Z

@@ -0,0 +1,131 @@
+//!! goog.Promise, unlike other implementations of Promise, has 2 template params
+//!! this test ensures that the specially handled TTE methods work for it as well


closure libs are on github - add a reference here for that.

rkirov · 2018-02-13T01:46:55Z

+          // TODO(lucassloan): goog.Promise has bad types that are coerced to any, so explicitly emit any
+          // and fix when the callers have been fixed.
+          if (className.equals("ಠ_ಠ.clutz.goog.Promise")) {
+            return "resolve < T >(value: " + className + " < T , any > | T): any;";


OOC, what happens if you do : <T, any> for the return type too?

That's the correct type, and all the bad user's code is surfaced.

I see, expand the comment so it is even clear what is the future state here - the correct type here would be (value: googPromise< T , any > | T): googPromise<T, any>;

rkirov · 2018-02-13T01:49:20Z

      String templateVarName = templateTypeNames.next();

-      String classTemplatizedType = className + " < RESULT >";
+      // TODO(lucassloan): goog.Promise has bad types that are coerced to any, so explicitly emit any


expand 'bad types' -> because of change from 1 type param to 2.

rkirov · 2018-02-13T01:59:16Z

          "--noImplicitAny",
-          "--strictNullChecks");
+          "--strictNullChecks",
+          "--noStrictGenericChecks");


add a comment next to this flag, similar to the one in the cl description. Long-term we should attempt removing this.

rkirov · 2018-02-13T02:00:57Z

  class PartialDeferred_Instance < VALUE = any > extends ಠ_ಠ.clutz.Base < VALUE > {
    constructor ( ) ;
-    then < RESULT > (opt_onFulfilled ? : ( (a : VALUE ) => PartialDeferred < RESULT > | RESULT ) | null , opt_onRejected ? : ( (a : any ) => any ) | null) : PartialDeferred < RESULT > ;
+    then < RESULT > (opt_onFulfilled ? : ( (a : VALUE ) => ಠ_ಠ.clutz.module$exports$tte$Promise$Partial.PartialDeferred < RESULT > | RESULT ) | null , opt_onRejected ? : ( (a : any ) => any ) | null) : ಠ_ಠ.clutz.module$exports$tte$Promise$Partial.PartialDeferred < RESULT > ;


this looks wrong? where is ಠ_ಠ.clutz.module$exports$tte$Promise$Partial defined?

Right here. PartialDefered_Instance is extended by PartialDeferred, which are both in the ಠ_ಠ.clutz.module$exports$tte$Promise$Partial namespace.

I see, I guess I expected PartialDeferred but doesn't hurt to fully qualify.

rkirov · 2018-02-13T02:01:22Z

+      // TODO(lucassloan): goog.Promise has bad types that are coerced to any, so explicitly emit any
+      // and fix when the callers have been fixed.
+      String classTemplatizedType =
+          className.equals("ಠ_ಠ.clutz.goog.Promise") ? " any" : className + " < RESULT >";


unless I am missing something, there is no test for users of 'goog.Promise' only tests for the declaration?

LucasSloan

PTAL

LucasSloan · 2018-02-13T18:26:35Z

      String templateVarName = templateTypeNames.next();

-      String classTemplatizedType = className + " < RESULT >";
+      // TODO(lucassloan): goog.Promise has bad types that are coerced to any, so explicitly emit any


LucasSloan · 2018-02-13T18:26:43Z

+      // TODO(lucassloan): goog.Promise has bad types that are coerced to any, so explicitly emit any
+      // and fix when the callers have been fixed.
+      String classTemplatizedType =
+          className.equals("ಠ_ಠ.clutz.goog.Promise") ? " any" : className + " < RESULT >";


LucasSloan · 2018-02-13T18:27:29Z

+          // TODO(lucassloan): goog.Promise has bad types that are coerced to any, so explicitly emit any
+          // and fix when the callers have been fixed.
+          if (className.equals("ಠ_ಠ.clutz.goog.Promise")) {
+            return "resolve < T >(value: " + className + " < T , any > | T): any;";


That's the correct type, and all the bad user's code is surfaced.

LucasSloan · 2018-02-13T18:39:27Z

          "--noImplicitAny",
-          "--strictNullChecks");
+          "--strictNullChecks",
+          "--noStrictGenericChecks");


LucasSloan · 2018-02-13T18:40:37Z

@@ -0,0 +1,131 @@
+//!! goog.Promise, unlike other implementations of Promise, has 2 template params
+//!! this test ensures that the specially handled TTE methods work for it as well


LucasSloan · 2018-02-13T18:42:13Z

  class PartialDeferred_Instance < VALUE = any > extends ಠ_ಠ.clutz.Base < VALUE > {
    constructor ( ) ;
-    then < RESULT > (opt_onFulfilled ? : ( (a : VALUE ) => PartialDeferred < RESULT > | RESULT ) | null , opt_onRejected ? : ( (a : any ) => any ) | null) : PartialDeferred < RESULT > ;
+    then < RESULT > (opt_onFulfilled ? : ( (a : VALUE ) => ಠ_ಠ.clutz.module$exports$tte$Promise$Partial.PartialDeferred < RESULT > | RESULT ) | null , opt_onRejected ? : ( (a : any ) => any ) | null) : ಠ_ಠ.clutz.module$exports$tte$Promise$Partial.PartialDeferred < RESULT > ;


Right here. PartialDefered_Instance is extended by PartialDeferred, which are both in the ಠ_ಠ.clutz.module$exports$tte$Promise$Partial namespace.

rkirov

Open a clutz bug to track this too

rkirov · 2018-02-13T19:26:37Z

+          // TODO(lucassloan): goog.Promise has bad types that are coerced to any, so explicitly emit any
+          // and fix when the callers have been fixed.
+          if (className.equals("ಠ_ಠ.clutz.goog.Promise")) {
+            return "resolve < T >(value: " + className + " < T , any > | T): any;";


I see, expand the comment so it is even clear what is the future state here - the correct type here would be (value: googPromise< T , any > | T): googPromise<T, any>;

rkirov · 2018-02-13T19:27:39Z

  class PartialDeferred_Instance < VALUE = any > extends ಠ_ಠ.clutz.Base < VALUE > {
    constructor ( ) ;
-    then < RESULT > (opt_onFulfilled ? : ( (a : VALUE ) => PartialDeferred < RESULT > | RESULT ) | null , opt_onRejected ? : ( (a : any ) => any ) | null) : PartialDeferred < RESULT > ;
+    then < RESULT > (opt_onFulfilled ? : ( (a : VALUE ) => ಠ_ಠ.clutz.module$exports$tte$Promise$Partial.PartialDeferred < RESULT > | RESULT ) | null , opt_onRejected ? : ( (a : any ) => any ) | null) : ಠ_ಠ.clutz.module$exports$tte$Promise$Partial.PartialDeferred < RESULT > ;


I see, I guess I expected PartialDeferred but doesn't hurt to fully qualify.

Clutz hard codes translations for TTE types, and goog.Promise has 2 type parameters, which wasn't previously accounted for in the translations. Had to turn on noStrictGenericChecks to make it compile, ts doesn't like extending promise-y things.

LucasSloan requested a review from rkirov February 13, 2018 00:33

googlebot added the cla: yes label Feb 13, 2018

LucasSloan force-pushed the better-goog-promise-typing branch from 8e7cab3 to 6acf839 Compare February 13, 2018 00:49

rkirov reviewed Feb 13, 2018

View reviewed changes

LucasSloan commented Feb 13, 2018

View reviewed changes

rkirov approved these changes Feb 13, 2018

View reviewed changes

Improve the typings for goog.Promise.

2ba0534

Clutz hard codes translations for TTE types, and goog.Promise has 2 type parameters, which wasn't previously accounted for in the translations. Had to turn on noStrictGenericChecks to make it compile, ts doesn't like extending promise-y things.

LucasSloan force-pushed the better-goog-promise-typing branch from b39f93e to 2ba0534 Compare February 13, 2018 19:51

LucasSloan merged commit 0f733de into master Feb 13, 2018

teppeis mentioned this pull request Dec 10, 2018

Return types on Promise #806

Open

teppeis mentioned this pull request Jan 14, 2019

Improve Promise and remove --noStrictGenericChecks #830

Closed

		@@ -0,0 +1,131 @@
		//!! goog.Promise, unlike other implementations of Promise, has 2 template params
		//!! this test ensures that the specially handled TTE methods work for it as well

Conversation

LucasSloan commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucasSloan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants