fix(database): call cancellation callback when using ref.on

[nealmanaktola]

Description

The cancellation/error callback is not being triggered using the following api

ref.on('value', (val) => ... , (err) => { ... })

Digging through the code, I noticed the listener is being wrapped but not called. I noticed there is a test for this...but not sure how that is passing.

Related issues

Fixes #5370

Release Summary

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [X ] Yes
• My change supports the following platforms;
  • Android
  • iOS
• My change includes tests;
  • e2e tests added or updated in packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • [ x] No

Test Plan

---

Think react-native-firebase is great? Please consider supporting the project with any of the below:

• 👉 Star this repo on GitHub ⭐️
• 👉 Follow React Native Firebase and Invertase on Twitter

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/CRD2ynmyE44riTFKpfzrEXaDuqoG
✅ Preview: https://react-native-firebase-git-fork-nealmanaktola-f-2d5ecd-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/FXLBkHXEsLU7t5ZmRcUYbnNcNsSD
✅ Preview: Failed

[codecov]

Codecov Report

Merging #5371 (9e23200) into master (3fef777) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 9e23200 differs from pull request most recent head 5c67b52. Consider uploading reports for the commit 5c67b52 to get more accurate results

@@           Coverage Diff           @@
##           master    #5371   +/-   ##
=======================================
  Coverage   74.56%   74.56%           
=======================================
  Files          96       96           
  Lines        4287     4287           
  Branches      921      921           
=======================================
  Hits         3196     3196           
  Misses       1021     1021           
  Partials       70       70           

[mikehardy]

As mentioned in comment with details, I don't think this is an error here, and I don't think this is needed. I think there is something else going on, possibly project-specific.

[nealmanaktola]

@mikehardy Gentle reminder if you had some time to look at this again

I also played around with the tests.

The test that was skipped https://github.com/invertase/react-native-firebase/blob/master/packages/database/e2e/query/on.e2e.js#L111 is failing w/o my patch.

After applying the patch, the test does in fact pass.

I do not believe it is project specific.

[mikehardy]

Sorry for the delay, had my mom in town from out of the country for the week 😁, this is in my queue and I do really appreciate the test investigation. We'll get this nailed down

[photorouletteadmin]

We faced the same problem. Patched it like this:

--- a/node_modules/@react-native-firebase/database/lib/DatabaseSyncTree.js
+++ b/node_modules/@react-native-firebase/database/lib/DatabaseSyncTree.js
@@ -263,7 +263,7 @@ class DatabaseSyncTree {
 
     if (once) {
       const subscription = SharedEventEmitter.addListener(eventRegistrationKey, event => {
-        this._onOnceRemoveRegistration(eventRegistrationKey, listener, event);
+        this._onOnceRemoveRegistration(eventRegistrationKey, listener)(event);
         subscription.remove();
       });
     } else {

I think this error was introduced by this commit: 5eb2f59. Looks like some of the other callbacks are also not being called.

[mikehardy]

Believe it or not (and you may not as this has sat a while) this is a high priority to fix for me and I'm sorry (for @nealmanaktola ) that I haven't reviewed it and gotten it worked into a release yet - had quite a few things going on unfortunately.

This will be resolved though, hopefully sooner than later

[mikehardy]

@nealmanaktola thanks again for making this PR, and thanks @photorouletteadmin - I think your one-liner is also correct but easier to read so I've updated the PR with it.
Pending CI this looks good to go and I'll do a release once it's green
@photorouletteadmin you mentioned you thought other callbacks weren't being called, I scanned the rest of the commit that created this issue (it was mine! sorry) and it seems like this was the only one affected but I'd be happy to merge anything else if you find something

[mikehardy]

Good to go!