Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency between docs and example regarding completionHandler() #244

Open
xsamix opened this issue Dec 3, 2020 · 6 comments
Open

Comments

@xsamix
Copy link

xsamix commented Dec 3, 2020

Call to completionHandler() and code around are originally from here: https://github.com/react-native-push-notification-ios/push-notification-ios/pull/104/files

Here completionHandler() call is removed from README.md and example AppDelegate.m: https://github.com/react-native-push-notification-ios/push-notification-ios/pull/207/files

Here it is added back to example AppDelegate.m, but README.md is left missing: https://github.com/react-native-push-notification-ios/push-notification-ios/pull/241/files

Last pull addresses completion handling and is included in newest release, so this must be meaningful.

So my question to @Naturalclar is, which way should it be? Presumably example is correct and docs are lagging, but...

@leethree
Copy link

yes i've found the same issue. it's very confusing because completionHandler is supposed to be called.

@apfritts
Copy link
Contributor

@leethree @xsamix you can't call the completionHandler() directly from the AppDelegate file because it will call before your JavaScript code executes (in my case anyway), especially if you do any asynchronous processing in your JavaScript handler. I believe this is actually a bug in the native code of this library and would love you to try out the updated version and let me know if it fixes your problem.

@leethree
Copy link

@apfritts i'm not familiar with iOS, but my understanding it that completionHandler should eventually be called (not necessarily in AppDelegate). is it correct?

the README doesn't give any guidance on that 🤷🏼

@apfritts
Copy link
Contributor

@leethree by calling notification.finish() from Javascript, it should have been calling the completionHandler but wasn't. My PR fixes that so you shouldn't call completionHandler from AppDelegate.m and instead make sure your notification handler is calling the notification's finish method.

In my instance, if I call the completionHandler from the AppDelegate.m file, the actual Javascript notification code runs afterwards which means iOS may kill the app before your code runs.

@leethree
Copy link

@apfritts I see. It makes sense to me 👍🏼

@malgorzatatanska
Copy link

@apfritts is your PR merged?
I use @react-native-community/push-notification-ios": "1.10.0" should your changes be available in this version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants