invertase / react-native-notifee

Moved to https://github.com/invertase/notifee
https://invertase.io/blog/open-sourcing-notifee
Other
465 stars 31 forks source link

[IOS] In-app notification behaviour determined by android channel config #297

Closed antoinefBam closed 3 years ago

antoinefBam commented 3 years ago

Hi again,

For some reasons I had to do some platform specific code for my notification module. I ended up with two files each calling notifee.displayNotification but with a different payload depending on the platform IOS or Android. At first I left a simple android: {} in the file dedicated to IOS but it made the in-app notifications disappear on IOS. The only way to make them work while leaving an android key was to set some android channel id.

For information, removing the android sub-object completely also works, but I find it disturbing that a configuration explicitly dedicated to Android can have any effect on the IOS side 😅

helenaford commented 3 years ago

oh interesting 🤔 we can see if we can fix this but we need some more details if you can share them. Can you paste the payload you were passing to displayNotification before you had to split it up based on platform?

helenaford commented 3 years ago

There could be a quick fix where we can ignore any validation for the other platform that's currently not in operation but can see pros and cons to this.

When you're developing it's great to know you have both platforms configured correctly to a certain extend without having to run on both devices, but that's probably a minor pro.

The cons are like what you described, where it becomes an issue if you don't have the information during runtime for that particular platform. Is that along the correct track? I can look into pushing up a fix.

antoinefBam commented 3 years ago

Here is a minimal example I just tested to reproduce the bug.

  // This makes in-app notification visible on IOS device
  await notifee.displayNotification({
    title: 'Hello',
    body: 'World',
    data: { some: 'data' },
    ios: {
      sound: 'default',
      badgeCount: 1,
    },
    android: {
      channelId: 'any-string-will-work-here',
    },
  });

  // This makes in-app notification invisible on IOS device
  await notifee.displayNotification({
    title: 'Hello',
    body: 'World',
    data: { some: 'data' },
    ios: {
      sound: 'default',
      badgeCount: 1,
    },
    android: {},
  });

  // This makes in-app notification visible on IOS device
  await notifee.displayNotification({
    title: 'Hello',
    body: 'World',
    data: { some: 'data' },
    ios: {
      sound: 'default',
      badgeCount: 1,
    },
  });
helenaford commented 3 years ago

OK. I can see why it fails because android expects channelId, but I agree it shouldn't really matter if you send an empty object or not, it should be consistent (always fail). Would changing the validation so if you're on iOS, anything to do with android is ignored, be a suitable solution to your issue?

antoinefBam commented 3 years ago

OK. I can see why it fails because android expects channelId, but I agree it shouldn't really matter if you send an empty object or not, it should be consistent (always fail). Would changing the validation so if you're on iOS, anything to do with android is ignored, be a suitable solution to your issue?

Yes I think it would be good in the sense that current behavior can lead to unexpected behavior.

However, this is not the most urgent thing to do since I already have a workaround in my case.

helenaford commented 3 years ago

OK sure. thank you for bringing it to our attention and sorry it caused you to separate your code out. I've highlighted it as a bug so it'll be fixed soon, probably in the next release. 💯

antoinefBam commented 3 years ago

Nice ! Thanks for the quick response 🙂

mikehardy commented 3 years ago

I can see another pro for allowing any non-current-platform blocks to be ignored - if an app is doing a data-transfer audit and minimizing transfer they'll eliminate extra data traffic by pruning unnecessary blocks, and users will benefit (most where I live are on pre-paid plans and they watch their megas like hawks!)

For DX I think non-current-platform validation should still be done - you had a great point there Helena about consistency during integration - and just emitted as debug logs in debug mode only though, as it's a big con to not have consistent validation during development / integration I think?

helenaford commented 3 years ago

Ah really? ha I just upgraded my 20gb to unlimited so I'm not watching mine. But yeah, it would help performance too.

Yeah, glad you suggested logs, I was thinking something along those lines. Probably easy enough to switch out errors with logs 😎 And I have faced this myself, so I can see the pain-point here which out weighs the the con for doing it.