onmyway133 / PushNotifications

🐉 A macOS, Linux, Windows app to test push notifications on iOS and Android
https://onmyway133.com
Other
2.26k stars 176 forks source link

Fix Android payload structure to let device receive proper push notification. #52

Closed buleglay closed 3 years ago

Ariandr commented 3 years ago

Hi @buleglay Thanks for PR. Have you tested the update yourself?

buleglay commented 3 years ago

Hi @buleglay Thanks for PR. Have you tested the update yourself?

Hi @Ariandr,

Thanks for checking.

The change has been tested on an OnePlus phone.

The issue was that Android devices won't be able to properly handle the push notification if both title & message was placed inside data object therefore nothing would show up.

According to this Firebase doc: https://firebase.google.com/docs/cloud-messaging/http-server-ref#downstream-http-messages-json, data is used for custom key-value pairs, while notification is for user-visible key-values pairs (in this case, title and message).

Hope it makes sense.

Regards, Sean

Ariandr commented 3 years ago

@buleglay It looks good to me, though I don't have Android device or environment to test it myself.

Hi @onmyway133 What do you think? I believe we can merge it, because the changes are pretty straightforward.

buleglay commented 3 years ago

@buleglay It looks good to me, though I don't have Android device or environment to test it myself.

Hi @onmyway133 What do you think? I believe we can merge it, because the changes are pretty straightforward.

This won’t be a “breaking change” to the existing users who’s currently relying on it to test their push notifications.

As they will still get the same data property containing the payload they’re currently sending, plus an additional notification property which would trigger native level push notifications.

Ariandr commented 3 years ago

@buleglay I agree. If @onmyway133 confirms, I will create a new release.

onmyway133 commented 3 years ago

Thanks for the PR @buleglay and thanks for checking @Ariandr , I've checked the changes and it looks good to me 👍

Ariandr commented 3 years ago

@onmyway133 Great. I merged it and will create a new release🙂

Ariandr commented 3 years ago

@buleglay Have a look at the release version please: https://github.com/onmyway133/PushNotifications/releases/tag/1.7.8

buleglay commented 3 years ago

LGTM!

Thanks guys! @Ariandr @onmyway133