[Bugfix] Ensure notification payload is passed to completion handler#10860
[Bugfix] Ensure notification payload is passed to completion handler#10860npomfret wants to merge 1 commit intofacebook:masterfrom
Conversation
|
By analyzing the blame information on this pull request, we identified @JAStanton and @corbt to be potential reviewers. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
| } | ||
|
|
||
| [self sendEventWithName:@"remoteNotificationReceived" body:notification.userInfo]; | ||
| [self sendEventWithName:@"remoteNotificationReceived" body:remoteNotification]; |
There was a problem hiding this comment.
👍 this bug is result of a 9 month long PR with subtle refactors over long periods of time and not testing after each refactor. Thank you for catching this, I believe what you have here is correct
There was a problem hiding this comment.
Cool. will it go into 38?
There was a problem hiding this comment.
¯_(ツ)_/¯ I'm a contributor not an owner, I don't have merge control :)
|
Can you describe what problem this PR is addressing? |
|
Sure, RN0.38 introduces a new feature that allows the user to call an iOS completion handler after processing a remote push notification. It's a great feature, without it there's no way to tell the OS that you've done the work to process a push notification. The OS will penalise your app and will eventually stop delivering push notifications. I think some refactor wasn't quite tested and the push notification object that gets delivered to the RN app is of the wrong type. In dev mode you get a red error screen and the notification object doesn't have the expected data in it. Basically, all iOS remote push notifications are broken in 0.38-rc. This fix results in the correct push notification object getting delivered to the app. Its just a one-liner - should be easy for someone to test. |
|
|
|
Yes i tested (of course!). Before the change, when an app receives a remote push notification you get a red error screen and the notification object delivered to the JS code doesn't contain the notification data. After the change it works as expected. I just meant it would be easy someone else to check... my first rn PR you see. |
|
No worries @npomfret, thanks for submitting the fix! We're just getting some more additional information to help us understand the scope and impact of the PR. |
|
@facebook-github-bot shipit |
|
Ah - @npomfret - just one more thing to do before we can import this. |
|
@npomfret Thanks again, I feel foolish for letting this slip by, thanks for testing it and fixing it :) |
|
Waiting on CLA to be signed before we can import this. |
|
What's a CLA? Is there anything I need to do? |
|
See the second reply: https://code.facebook.com/cla |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Hi, I just checked the 38 branch and this PR still isn't present. Was wondering if there's a hold-up. Is there anything else I need to do to get it in there? (Sorry, not familiar with the workflow here yet) |
|
Nope @npomfret, cherry-picking it now and releasing stable :) |
Fixes #10863