React-native-firebase: [🐛] Type Definition Error in database Types

[SCasarotto]

Issue

The type definition for database Query.on has an incorrect type.

Current Type:

on(
      eventType?: EventType,
      callback?: Function,
      cancelCallbackOrContext?: Record<string, any>,
      context?: Record<string, any> | null,
    ): Function;

cancelCallbackOrContext is describe as : @param cancelCallbackOrContext An optional callback that will be notified if your event subscription is ever canceled because your client does not have permission to read this data (or it had permission but has now lost it). This callback will be passed anErrorobject indicating why the failure occurred. but has a type of Record<string, any>.

I believe this type should be similar to Query.once's failureCallbackContext which is ((a: Error) => void) | Record<string, any> | null. I do wonder why either of them is a Record in the first place if they are just meant as a function but I haven't been working with the lib in ts for more than a few minutes now (converted js -> ts).

Happy to make a quick PR but thought I would post here before assuming the right answer.

[SCasarotto]

Additionally, should callback be typed more like once's successCallback ?

?: (a: DataSnapshot, b?: string | null) => any

[mikehardy]

Hi there!
For APIs with analogs in firebase-js-sdk, firebase-js-sdk is our basis for API shape, and this should be the comparison:

https://firebase.google.com/docs/reference/js/firebase.database.Query#on

on ( 
eventType :  EventType ,  
callback :  ( a :  DataSnapshot ,  b ? :  string | null ) => any ,  
cancelCallbackOrContext ? :  Object | null ,  
context ? :  Object | null ) : ( a :  DataSnapshot | null ,  b ? :  string | null ) => any

https://firebase.google.com/docs/reference/js/firebase.database.Query#once

once ( 
eventType :  EventType ,  
successCallback ? :  ( a :  DataSnapshot ,  b ? :  string | null ) => any , 
 failureCallbackOrContext ? :  ( ( a :  Error ) => void ) | Object | null ,  
context ? :  Object | null ) : Promise < DataSnapshot >

Here is the direct code reference

https://github.com/firebase/firebase-js-sdk/blob/5b2d1073a7a4e1366b0d807510f2bded713ed7bc/packages/database-types/index.d.ts#L84-L95

on(
    eventType: EventType,
    callback: (a: DataSnapshot, b?: string | null) => any,
    cancelCallbackOrContext?: ((a: Error) => any) | Object | null,
    context?: Object | null
  ): (a: DataSnapshot, b?: string | null) => any;
  once(
    eventType: EventType,
    successCallback?: (a: DataSnapshot, b?: string | null) => any,
    failureCallbackOrContext?: ((a: Error) => void) | Object | null,
    context?: Object | null
  ): Promise<DataSnapshot>;

Without actually doing the comparison, is there actually a typing inconsistency between @react-native-firebase/database and firebase-js-sdk? If there is, then a PR to re-shape would be appreciated, otherwise I believe we're meeting the intended API contract

[SCasarotto]

Hey Appreciate you getting back to me @mikehardy .

The docs show a different type than the type definition file. Specifically:
cancelCallbackOrContext ? : Object | null ,
vs
cancelCallbackOrContext?: ((a: Error) => any) | Object | null,

I have just submitted a feedback request to the firebase docs site to get them to match but we should update them here to match that actual file. I will submit a PR to do so.

[mikehardy]

@SCasarotto thank you, and especially thank for doing the work upstream to weld things together, and then to conform here with reality (the actual file), no way I would have time but the outcome (doc fix + code fix) will help everyone. Kudos

[stale[bot]]

Hello 👋, to help manage issues we automatically close stale issues.
This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs.
Thank you for your contributions.

[SCasarotto]

This issue is not stale. The issue is that the website docs still don't match the code. Not sure when it will be resolved as the submit feedback is unidirectional. If I don't see a change next time this bot appears I may just submit feedback again. Unless, @mikehardy are you aware of another way to propose fixes to the firebase docs?

[mikehardy]

Yeah, you just hit the edit button on the top right of every page, and let github walk you through the documentation PR workflow for a PR to the docs. We merge things quickly and regularly, join us :-) https://github.com/invertase/react-native-firebase/pulse/monthly

[SCasarotto]

I am referring to these docs: https://firebase.google.com/docs/reference/js/firebase.database.Query#on

[mikehardy]

Oh my goodness - I was processing things from email (a bad habit I guess) so I lacked context and thought this was our 90% occurrence of just a quick PR to our docs. Sorry. You are correct, it's unidirectional. I'm not sure of another way to add feedback for docs but you might get attention back-channel if you go to firebase-js-sdk repo and file an issue related to the method signature asserting it is wrong vs the docs. No guarantee, just a thought

[SCasarotto]

Thats a good thought. I may try to do that later this week. Thanks!

[SCasarotto]

Progress is being made. https://github.com/firebase/firebase-js-sdk/issues/4250#issuecomment-760473649