react-native-onesignal icon indicating copy to clipboard operation
react-native-onesignal copied to clipboard

Published 20 hours ago • verified publisher icon OneSignal

Fix sendEvent fired before React was fully set up

Open abdullahizzuddiin opened this issue 5 years ago • 8 comments

Bug

The application crashed after received notification when application is still starting up (the React instance had not been fully set up)

Caused by java.lang.RuntimeException
Tried to access a JS module before the React instance was fully set up. Calls to ReactContext#getJSModule should only happen once initialize() has been called on your native module.

Resolved By

Add react instance checker before emit event. Only emit event, when react instance was active

Fixes #877

This change is Reviewable

abdullahizzuddiin avatar Nov 29 '19 10:11 abdullahizzuddiin

Hi @abdullahizzuddiin , any plan to include this fix in recent SDK release?

conradchenghk01 avatar Nov 10 '20 07:11 conradchenghk01

Thanks for submitting this PR. Has anyone found official docs regarding this or an ongoing bug in React Native related to this issue? In other words, is this a workaround or a temporary solution?

I found a similar related thread but it isn't official React Native so I want to be sure this change is the correct fix for the problem.

Update: I found an example of this being used here

While adding this check may patch the issue, we should investigate why this is happening in the first place

rgomezp avatar Nov 11 '20 00:11 rgomezp

Thanks for submitting this PR. Has anyone found official docs regarding this or an ongoing bug in React Native related to this issue? In other words, is this a workaround or a temporary solution?

Thanks @rgomezp , from crashlytics, this issue is still growing, so we wonder this workaround / fix can be apply in coming sdk release.

conradchenghk01 avatar Nov 11 '20 01:11 conradchenghk01

Hi OneSignal administrator, any plan to include this fix to Onesignal 4.0 sdk? We got a spike related to this incident.

conradchenghk01 avatar Dec 08 '20 01:12 conradchenghk01

Howdy @conradchenghk01 , I am sorry to hear you're having spikes in crashes regarding this.

I will bring up to my team this week to prioritize.

rgomezp avatar Jan 12 '21 00:01 rgomezp

Is this PR enough to solve this issue or you will create another solution? @rgomezp

Actually, I never seen this crash again after my app use this patch.

abdullahizzuddiin avatar Jan 12 '21 00:01 abdullahizzuddiin

Thanks @rgomezp , same as @abdullahizzuddiin , we applied a patch to solve the issue too~

conradchenghk01 avatar Jan 12 '21 03:01 conradchenghk01

Thanks @abdullahizzuddiin, we are using this patch to workaround this issue too.

I noticed that the SDK is already doing something similar to this in RNPushNotificationJsDelivery, maybe this info can speed up the process to merge this PR? Thanks!

mussegam avatar Jul 02 '21 10:07 mussegam