Typings: Add Observer interface and improve typings for Auth, Storage, and Messaging #56

jsayol · 2017-06-15T21:42:15Z

Discussion

Fixes issue #54

Testing

n/a

API Changes

n/a

jsayol · 2017-06-15T21:50:59Z

Out of curiosity: these typing files seem to be auto-generated. Was that a one-time thing, or is there a process to regenerate them when there's changes to the API?

bojeil-google · 2017-06-16T06:06:03Z

typings/app.d.ts

@@ -123,7 +123,7 @@ declare namespace firebase.auth {
    currentUser : firebase.User | null ;
    fetchProvidersForEmail (email : string ) : firebase.Promise < any > ;
    getRedirectResult ( ) : firebase.Promise < any > ;
-    onAuthStateChanged (nextOrObserver : Object , error ? : (a : firebase.auth.Error ) => any , completed ? : ( ) => any ) : ( ) => any ;
+    onAuthStateChanged (nextOrObserver : Object | ((user: firebase.User | null) => any), error ? : (a : firebase.auth.Error ) => any , completed ? : ( ) => any ) : ( ) => any ;
    onIdTokenChanged (nextOrObserver : Object , error ? : (a : firebase.auth.Error ) => any , completed ? : ( ) => any ) : ( ) => any ;


While you're at it, can you make the same change here too?

Good point. I'll check if there's any other places too.

How about also introducing an Observer interface to use instead of the more generic Object?

interface Observer { next?: (user: firebase.User | null) => any; error?: (error: firebase.auth.Error) => any; complete?: () => any; }

bojeil-google · 2017-06-16T06:06:18Z

typings/firebase.d.ts

@@ -123,7 +123,7 @@ declare namespace firebase.auth {
    currentUser : firebase.User | null ;
    fetchProvidersForEmail (email : string ) : firebase.Promise < any > ;
    getRedirectResult ( ) : firebase.Promise < any > ;
-    onAuthStateChanged (nextOrObserver : Object , error ? : (a : firebase.auth.Error ) => any , completed ? : ( ) => any ) : ( ) => any ;
+    onAuthStateChanged (nextOrObserver : Object | ((user: firebase.User | null) => any), error ? : (a : firebase.auth.Error ) => any , completed ? : ( ) => any ) : ( ) => any ;
    onIdTokenChanged (nextOrObserver : Object , error ? : (a : firebase.auth.Error ) => any , completed ? : ( ) => any ) : ( ) => any ;


bojeil-google

The auth changes look fine. You will need someone from messaging and storage to confirm the other changes.

jshcrowthe · 2017-06-16T17:15:54Z

@gauntface Can you also take a look at this?

sphippen

Looks like a straightforward improvement, just one comment.

sphippen · 2017-06-27T21:36:09Z

typings/app.d.ts

@@ -76,6 +76,12 @@ declare namespace firebase {
    uid : string ;
  }

+  interface Observer<V, E> {
+    next?: (value: V | null) => any;


In a few places inside the SDK, these are typed with a void return value (

firebase-js-sdk/src/app/subscribe.ts

Line 16 in 657055d

export type NextFn<T> = (value: T) => void;

,

firebase-js-sdk/src/storage/implementation/observer.ts

Line 18 in 657055d

type NextFn<T> = (value: T) => void;

). Let's make the return types here "void" instead of "any" for consistency.

(note: also applies to the return types in the function signatures below)

jsayol · 2017-06-28T07:26:25Z

Should these same changes be reflected in the type annotation comments of the externs files?

sphippen · 2017-06-28T17:57:56Z

Hmm, forgot about those. As far as I can tell we've been keeping the TS<->Closure typings in sync, so the Closure externs files should also be updated.

jsayol · 2017-06-28T19:43:43Z

@sphippen I just pushed the changes to the externs to get them in sync with the typings. I'm not extremely familiar with Closure compiler annotations so make sure to review those carefully, please :)

I've also made a few more improvements to the typings while I was at it. I added the NextFn, ErrorFn, and CompleteFn types (they were already present in the externs) and I modified the signature of the firebase.storage.UploadTask#on() method to reflect the two different usages.

sphippen · 2017-06-29T22:00:05Z

externs/firebase-app-externs.js

+
+firebase.Observer.prototype.complete = function() {};
+
+/**


Unfortunately Closure doesn't support templated typedefs:

../../externs/firebase-app-externs.js:296: WARNING - Bad type annotation. type annotation incompatible with other annotations. See https://github.com/google/closure-compiler/wiki/Bad-Type-Annotation for more information. * @typedef {function(?V): void} ^ ../../externs/firebase-app-externs.js:298: WARNING - Misplaced template annotation. @template is only allowed in class, constructor, interface, function or method declarations firebase.NextFn; ^^^^^^^^^^^^^^^ ../../externs/firebase-app-externs.js:302: WARNING - Bad type annotation. type annotation incompatible with other annotations. See https://github.com/google/closure-compiler/wiki/Bad-Type-Annotation for more information. * @typedef {function(!E): void} ^ ../../externs/firebase-app-externs.js:304: WARNING - Misplaced template annotation. @template is only allowed in class, constructor, interface, function or method declarations firebase.ErrorFn; ^^^^^^^^^^^^^^^^

It's probably best to leave the NextFn and ErrorFn arguments as raw "!function(...)" annotations instead of adding the typedefs here. I'd be fine if you left CompleteFn and Unsubscribe in as typedefs and added a comment about the templated typedefs being unfeasible or you just removed all of them. Either one is good with me.

Ah well, too bad. I removed the templates from NextFn and ErrorFn and moved the typedefs back to the INTERNAL file, where they're still used. I replaced all the other uses with the original !function(...) annotations.

sphippen

Just one last change on the Closure externs that I missed the first time, then this all looks good to me.

sphippen · 2017-07-01T00:01:53Z

externs/firebase-app-externs.js

+
+/**
+ * @template V
+ * @param {?V} value


Just one last change, put the "@template V" and "@template E" annotations in the same comment block as the "@interface" instead of on the individual methods.

Like this?

/** * @template V, E * @interface **/ firebase.Observer = function() {}; /** * @param {?V} value */ firebase.Observer.prototype.next = function(value) {}; /** * @param {!E} error */ firebase.Observer.prototype.error = function(error) {}; firebase.Observer.prototype.complete = function() {};

sphippen · 2017-07-05T20:40:49Z

Awesome, looks good to me.
Also @jsayol, re: automatic generation of the typings, they have always been maintained by hand. If I remember correctly the TypeScript typings were based on a machine-converted form of the Closure externs files, but we had to do some manual cleanup afterward and have been maintaining them by hand since.

Thanks for the PR!

And since @bojeil-google and @gauntface have given their OK on this I'll go ahead and merge the PR now.

jshcrowthe requested a review from bojeil-google June 16, 2017 05:07

bojeil-google suggested changes Jun 16, 2017

View reviewed changes

jsayol force-pushed the fix-issue-54 branch from 4e3a61e to 1ddb382 Compare June 16, 2017 07:37

jsayol changed the title ~~fix(auth): improve typings for onAuthStateChanged~~ fix(auth): improve typings for Auth methods Jun 16, 2017

jsayol changed the title ~~fix(auth): improve typings for Auth methods~~ Typings: Add Observer interface and improve typings for Auth, Storage, and Messaging Jun 16, 2017

bojeil-google reviewed Jun 16, 2017

View reviewed changes

bojeil-google requested a review from sphippen June 16, 2017 17:13

jshcrowthe requested a review from gauntface June 16, 2017 18:38

sphippen suggested changes Jun 27, 2017

View reviewed changes

jsayol force-pushed the fix-issue-54 branch from 58d8526 to 78e1133 Compare June 27, 2017 23:44

gauntface approved these changes Jun 28, 2017

View reviewed changes

sphippen approved these changes Jun 28, 2017

View reviewed changes

jsayol force-pushed the fix-issue-54 branch from 78e1133 to 235f061 Compare June 28, 2017 19:40

sphippen suggested changes Jun 29, 2017

View reviewed changes

jsayol force-pushed the fix-issue-54 branch from 235f061 to 52ff208 Compare June 29, 2017 22:38

sphippen suggested changes Jul 1, 2017

View reviewed changes

fix(core): improve typings with new Observer interface

eb51843

jsayol force-pushed the fix-issue-54 branch from 52ff208 to eb51843 Compare July 1, 2017 00:13

sphippen approved these changes Jul 5, 2017

View reviewed changes

sphippen merged commit 6aaef70 into firebase:master Jul 5, 2017

jshcrowthe mentioned this pull request Jul 6, 2017

nextOrObserver:Object type definition in typescript doesn't allow type inference #54

Closed

jsayol deleted the fix-issue-54 branch July 13, 2017 07:21

jsayol mentioned this pull request Jul 26, 2017

feat(auth): Adds support to specifying Auth state persistence. #105

Merged

firebase locked and limited conversation to collaborators Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typings: Add Observer interface and improve typings for Auth, Storage, and Messaging #56

Typings: Add Observer interface and improve typings for Auth, Storage, and Messaging #56

jsayol commented Jun 15, 2017

jsayol commented Jun 15, 2017

bojeil-google Jun 16, 2017

jsayol Jun 16, 2017

bojeil-google Jun 16, 2017

jsayol Jun 16, 2017

bojeil-google left a comment

jshcrowthe commented Jun 16, 2017

sphippen left a comment

sphippen Jun 27, 2017

sphippen Jun 27, 2017

jsayol Jun 27, 2017

jsayol commented Jun 28, 2017

sphippen commented Jun 28, 2017

jsayol commented Jun 28, 2017

sphippen Jun 29, 2017

jsayol Jun 29, 2017

sphippen left a comment

sphippen Jul 1, 2017

jsayol Jul 1, 2017

jsayol Jul 1, 2017

sphippen commented Jul 5, 2017 •

edited

Loading

Typings: Add Observer interface and improve typings for Auth, Storage, and Messaging #56

Typings: Add Observer interface and improve typings for Auth, Storage, and Messaging #56

Conversation

jsayol commented Jun 15, 2017

Discussion

Testing

API Changes

jsayol commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bojeil-google left a comment

Choose a reason for hiding this comment

jshcrowthe commented Jun 16, 2017

sphippen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsayol commented Jun 28, 2017

sphippen commented Jun 28, 2017

jsayol commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphippen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphippen commented Jul 5, 2017 • edited Loading

sphippen commented Jul 5, 2017 •

edited

Loading