Update Observer and Messaging types to match internal types #786
Conversation
mmermerkaya
requested review from
gauntface,
mikelehen,
pinarx,
schmidt-sebastian and
wilhuff
as code owners
packages/firebase/index.d.ts
Outdated
| complete(): any; | ||
| error(error: E): any; | ||
| next(value: V | null): any; | ||
| interface Observer<T> { |
There was a problem hiding this comment.
this part of the change is breaking and we just shipped our 5.0.0 release.
You'll need to remove this portion (and other relevant edits).
For context: Users can annotate their TS code w/ firebase.Observer<any, any>. This change breaks that experience.
There was a problem hiding this comment.
It's not a breaking change, it's a bug fix. If they are annotating with any, then they are using wrong types and their application will not work as expected. In the implementation, ErrorFn takes an Error instance as its argument, not any. If they expect it to be any, they're going to have a bad time.
Changed it to a generic with a default value of Error, so at least users can do firebase.observer<any> (and second generic will default to Error) as well as firebase.Observer<any, any>.
There was a problem hiding this comment.
Disagree wholeheartedly that annotating with any is "using wrong types" and I can write a fully functional application using only any types throughout my app.
That said, I'm happy w/ the default Error type in the generic. Existing type annotations won't break and you support the single param generic use case which, altogether, is a better place to land.
Thanks!
There was a problem hiding this comment.
Sure you could, but if you tried to call .message on a number or a string, you'd get a runtime error :)
mmermerkaya
requested review from
bojeil-google,
sphippen and
wti806
as code owners
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
mmermerkaya
force-pushed
the
mmermerkaya-observer-messaging-types
branch
from
90a071a to
8f57b8d
Compare
|
CLAs look good, thanks! |
4 participants
No description provided.