[No QA] fix: null check for app state subscription added #6936
Conversation
MelvinBot
requested review from
TomatoToaster
and removed request for
a team
|
@TomatoToaster Before you merge, I would recommend checking the discussion on upgrading the RN in the linked issue. |
|
Also, if we want to merge this PR, it needs the |
|
|
| @@ -26,6 +26,9 @@ function addBecameActiveListener(callback) { | |||
| } | |||
| const appStateChangeSubscription = AppState.addEventListener('change', appStateChangeCallback); | |||
| return () => { | |||
| if (!appStateChangeSubscription) { | |||
| return; | |||
| } | |||
| appStateChangeSubscription.remove(); | |||
There was a problem hiding this comment.
I think this code change still makes sense regardless right? It might be redundant with the RN upgrade if that makes it so appStateChangeSubscription is never null, but it wouldn't hurt to have this check.
There was a problem hiding this comment.
Dope then I think it's safe to merge!
There was a problem hiding this comment.
FWIW, I think including the null check is a good idea even in RN 0.65+, because, while unlikely, I think we could potentially have a race condition like this if a component mounts and unmounts almost instantly:
- Constructor initializes
appStateChangeSubscriptiontonull. - Component mounts,
componentDidMountruns and starts creating the subscription. - Component unmounts and
componentWillUnmountruns beforecomponentDidMountfinishes creating the subscription. So the subscription is still null 💥
There was a problem hiding this comment.
Noted. Thanks for the explanation :)
[No QA] fix: null check for app state subscription added (cherry picked from commit b930390)
|
🚀 Deployed to staging by @TomatoToaster in version: 1.1.24-19 🚀
|
|
🚀 Deployed to production by @francoisl in version: 1.1.24-19 🚀
|
Details
Fixed Issues
$ #6932
Tests
QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android