-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix(messaging, ios): avoid hang on notification registration #7797
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Previously there were cases where notification registration requests would wait indefinitely for the choreography of "request registration from system", "wait for system to respond" completed. There are cases (specifically if you are missing permissions...) where this choreography will never complete. Now we have a delayed async task that waits (for 10 seconds currently) and if it runs and sees that the choreography wasn't complete it cleans up and rejects so you avoid infinite hangs See #7272
Related #7272 - the only difference between react-native-permissions and our requestPermission implementation was an immediate registration for notifications after a successful grant Assuming you would never request permission if you did not want to receive notifications, we now copy this behavior This allows messaging registration and APNS token tests to pass again on capable simulators (after altering tests to use the new permission behavior)
the idea of having an exercise of android emulator at our lowest supported API is temping, but it has not been stable in practice
5eb5b16
to
88616ba
Compare
during the change over to apple silicon, our ccache stopped actually being used during compilation, "no objects cached" brew paths are different between intel and apple silicon, so the old path addition we made was no longer correct it is possible to just ask the action to make symlinks for us, now we take advantage of that feature and /usr/local/bin has the links so they're in the path
they definitely work locally for me but it was not possible to quickly stabilize them in CI despite some effort. Disabling them there to get CI green and unblock merge / release progress
577a52f
to
adc4eea
Compare
The infinite hang is definitely fixed, and I am able to successfully run tests locally on simulators with apple silicon (with correct remote notification registration...) but I do want to note that I still was not seeing successful registration on apple silicon in github's action runners. Why? I really don't know - I tried very hard to determine why but couldn't get a root cause. I'm going to disable these tests in CI unfortunately, but they'll run locally Some further notes are that when running locally during development of these fixes, the permissions grants of an app are stateful, and apparently a bit racy. So testing is somewhat difficult, but I almost always had good results if I installed an app and requested permission, then restarted it and registered for remote notifications. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7797 +/- ##
==========================================
+ Coverage 32.97% 33.36% +0.39%
==========================================
Files 252 252
Lines 12530 12530
Branches 1954 1952 -2
==========================================
+ Hits 4131 4179 +48
+ Misses 8305 8262 -43
+ Partials 94 89 -5 |
Description
There has been a very troublesome issue with requests to register for notifications hanging infinitely in recent iOS versions
it was narrowed down by users (thank you!) to missing permissions, and even more specifically to a difference in behavior between requesting the permissions via react-native-permissions or the
requestPermission
method in this libraryTwo related fixes are present here to avoid the infinite hang in all cases, and to correctly function in the normal case:
requestPermissions
call will now immediately register for notifications, as that was the only difference between react-native-permissions and this libraryRelated issues
Fixes:
Release Summary
Conventional Commits followed on all commits. This is the way.
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Oh boy this required a lot of testing. Infinite runs of the e2e tests locally, should pass CI
I put a fix in for android CI as well to just run one API, we should have CI green completely for the first time in a while with these changes
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter