React-native-firebase: 5.0.0-rc1 - Notification.js problem

[NicholasBertazzonAga]

Issue

Hi,
I'm having some crashes when there's a notification coming in, using your latest 5.0 package. It's an undefined when getting a native module while testing it on Android.
So I went into Notification.js and modified it a bit:

Initial code:

constructor(nativeNotification, notifications) {
    if (nativeNotification) {
      this._body = nativeNotification.body;
      this._data = nativeNotification.data;
      this._notificationId = nativeNotification.notificationId;
      this._sound = nativeNotification.sound;
      this._subtitle = nativeNotification.subtitle;
      this._title = nativeNotification.title;
    }

      this._ios = new IOSNotification(this, notifications, nativeNotification && nativeNotification.ios); // Defaults
      this._android = new AndroidNotification(this, nativeNotification && nativeNotification.android);

    this._data = this._data || {}; // TODO: Is this the best way to generate an ID?

    this._notificationId = this._notificationId || generatePushID();
  }

Code with the check for the Platform I put in:

constructor(nativeNotification, notifications) {
    if (nativeNotification) {
      this._body = nativeNotification.body;
      this._data = nativeNotification.data;
      this._notificationId = nativeNotification.notificationId;
      this._sound = nativeNotification.sound;
      this._subtitle = nativeNotification.subtitle;
      this._title = nativeNotification.title;
    }

    if(Platform.OS === 'ios'){
      this._ios = new IOSNotification(this, notifications, nativeNotification && nativeNotification.ios); // Defaults
    }else{
      this._android = new AndroidNotification(this, nativeNotification && nativeNotification.android);
    }

    this._data = this._data || {}; // TODO: Is this the best way to generate an ID?

    this._notificationId = this._notificationId || generatePushID();
  }

Now, on Android, it doesn't crash anymore. Should it be like mine, or it shouldn't crash anyway?

Environment

1. Application Target Platform: Both, but tested only on Android

2. Development Operating System: macOS HIgh Sierra

3. React Native version: 0.57 rc3

4. React Native Firebase Version: 5.0.0-rc1

5. Firebase Module: notifications

6. Are you using typescript? no

---

Loving react-native-firebase? Please consider supporting them with any of the below:

• 👉 Back financially via Open Collective
• 👉 Follow React Native Firebase and Invertase on Twitter
• 👉 Star this repo on GitHub ⭐️

[Salakar]

Hey @NichAga

There's an issue at the moment like you've found, see: https://github.com/invertase/react-native-firebase/pull/1393#issuecomment-416523878

The workaround is the following:

[NicholasBertazzonAga]

Hi, @Salakar
I think I'm going to stay with my modification, because it has been working good, both on iOS and Android.

Also, I'm not making a new Notification by myself, but it's coming as a background notification.

Thanks! Waiting for the new update. You library rocks!

[JanErikFoss]

After adding a _Platform.OS_ check and using _new Notification(null, firebase.notifications())_ , I'm getting an exception on iOS. Android seems fine, though I still haven't been able to make the notification show in foreground.

The foreground notification is shown over the error screen.

[Salakar]

Hey, rc2 is up, please could you try it out, this should be resolved in there now. Thanks

[rahulgi]

@Salakar, I am still experiencing this issue on Android at 5.0.0-rc2 when receiving a remote notification after having registered an onNotification callback as in #1481.

Definitely can't claim any real understanding of how this library works (and thank you for all the work you do on it!) - however, from the bit of looking around I did:

The line of code that you mention in https://github.com/invertase/react-native-firebase/issues/1445#issuecomment-417666005 is still the issue. I'm not manually constructing the notification b/c it's coming from a remote source, and the workaround that @NichAga suggests does work for me. Why are we constructing an iOS notification on Android if the constructor can have native side effects?

[kkotelczuk]

I got this issue with React Native: 0.57 and react-native-firebase: 5.0.0-rc2, but only with foreground notifications.

[Salakar]

@rahulgi this bug was introduced by an external PR, specifically: https://github.com/invertase/react-native-firebase/pull/1393. As I didn't originally write the PR and the author is currently AWOL it's difficult for me to pin-point the exact issue without proper stack traces or a reproducable repo.

After debugging with someone on discord it appeared that my change had resolved the issue, but my guess after your feedback is that this is not the case. 😞

If you or anyone could assist me with a stack trace and perhaps using the debugger and have pause on uncaught exception etc showing me the stack there and the line it breaks on (code view) as well as the native notification payload/scope variables etc that'd be really helpful.

I can push another RC pretty quick once we can resolve this

[rahulgi]

@Salakar, happy to help, and thanks for the quick reply! Tried to do the things you asked for below, and let me know if there's any further information you want. Going to sleep now, but I can look further in the morning / maybe dig through that PR you referenced. Cheers!

Note: edited from original post b/c I think some console.logs I'd added messed with the source mapping and caused weird traces.

Screencap of source at caught exception:

Stack trace at exception:

console.trace
(anonymous) @ VM160:1
complete @ IOSNotification.js:33
IOSNotification @ IOSNotification.js:37
Notification @ Notification.js:27
(anonymous) @ index.js:65
emit @ EventEmitter.js:190
(anonymous) @ events.js:40
emit @ EventEmitter.js:190
__callFunction @ MessageQueue.js:344
(anonymous) @ MessageQueue.js:107
__guard @ MessageQueue.js:291
callFunctionReturnFlushedQueue @ MessageQueue.js:106
(anonymous) @ debuggerWorker.js:72

nativeNotification payload:
Not sure if there's a better way to do this / this is what you want, but I added a console.log to print out the nativeNotification variable in the JS at line 26 of Notification.js.

nativeNotification {
  "android": {},
  "title": "RNFirebase 5.0.0-rc2 bug demo",
  "notificationId": "0:1537371442806982%6add9f926add9f92",
  "data": {},
  "body": "Hi!"
}

[Salakar]

@rahulgi awesome thanks for that, straight away I can see the issue 👌 will get a fix and a new rc out.

[rahulgi]

@Salakar, much appreciated!

[Salakar]

@rahulgi @kkotelczuk @JanErikFoss @NichAga RC3 has just been published, please give it a go and let me know how it goes.

Thanks again @rahulgi for the assist 👍

[NicholasBertazzonAga]

@Salakar gonna test it right now, thanks!

EDIT:
It's working as expecting, I don't have the crash anymore. Thanks guys.

[Salakar]

@NichAga thanks for testing :)

Will close this issue now. 🎉

[rahulgi]

@Salakar, ditto, worked great! Thanks for the callout in the commit message too lol ^.^ feeling very honored for not doing much.

[leethree]

I'm still getting the same error with v5.0.0.

const notification = new firebase.notifications.Notification(
    undefined,
    firebase.notifications(),
  )
    .setTitle(title || '')
    .setBody(body || '')
    .setData(data)
  notifications.displayNotification(notification);

And I got:

Exception '*** -[__NSDictionaryM setObject:forKeyedSubscript:]: key cannot be nil' was thrown while invoking complete on target RNFirebaseNotifications with params (
    "<null>",
    1
)

[JanErikFoss]

@leethree Try removing the quickfix, just use

new firebase.notifications.Notification()

[huyhai]

same at "react-native-firebase": "^5.0.0-rc4"
key cannot be nil' was thrown while invoking complete on target RNFirebaseNotifications with params (
"",
1
)

[Salakar]

Use notifications as normal now on the full v5.0.0 release as per the docs. The 'quick fix' is no longer required

[petarivanov9]

Hello guys,
I am having the same problem on iOS with

"react": "16.5.0",
"react-native": "0.57.1",
"react-native-firebase": "^5.0.0",

Could anyone help me? @Salakar