Closed ifsnow closed 4 years ago
@matt-oakes I think this is related to your PR #666 merged by @janicduplessis - what do you guys think? I'm going to integrate this locally via patch-package to see if it mitigates the crashes I see (as described in #675)
In case anyone else wants to try it, this is relatively easy to paste into a file for patch-package consumption:
mike@isabela:~/work/Kullki/ksocialscore/packages/public-app (onboarding) % cat patches/react-native-fbsdk+1.1.1.patch
diff --git a/node_modules/react-native-fbsdk/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java b/node_modules/react-native-fbsdk/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java
index ee66e1d..bfdf5d7 100644
--- a/node_modules/react-native-fbsdk/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java
+++ b/node_modules/react-native-fbsdk/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java
@@ -47,9 +47,11 @@ public class FBAccessTokenModule extends ReactContextBaseJavaModule {
private final AccessTokenTracker accessTokenTracker = new AccessTokenTracker() {
@Override
protected void onCurrentAccessTokenChanged(AccessToken oldAccessToken, AccessToken currentAccessToken) {
- mReactContext
- .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
- .emit(CHANGE_EVENT_NAME, currentAccessToken == null ? null : Utility.accessTokenToReactMap(currentAccessToken));
+ if (mReactContext.hasActiveCatalystInstance()) {
+ mReactContext
+ .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
+ .emit(CHANGE_EVENT_NAME, currentAccessToken == null ? null : Utility.accessTokenToReactMap(currentAccessToken));
+ }
}
};
@ifsnow is your solution safe for production?
@vforvasile for what it's worth, after reviewing it myself it seemed correct, so I'm using it in production now via that patch I posted above (in combination with patch-package module to apply it for all devs + environments automagically) and it's working fine. No more crash reports.
@vforvasile Sure. As @mikehardy said, it works fine without any problem in production.
Can I send a PR with the changes you requested @janicduplessis ??
@ifsnow are you gonna update this PR??
@chakrihacker What update are you talking about? My app with this patch has been working well for months without any crashes.
Your PR works well and most of the native modules are following this way of checking, but @janicduplessis wanted a refactor with initializing and destroy methods so it would be nice
Yeah - @ifsnow mine is working also, but if it's not upstream or planned to go upstream, it's a dead PR and/or extra patch weight you have to carry. The idea is to upstream it so we don't crash and we don't have to carry a patch :-). Seems like @chakrihacker is up for it ?, in my world that mean "go for it @chakrihacker" :-)
@janicduplessis Can we merge this PR and @chakrihacker made another PR to do this refactor and release another version of this library? This PR is opened for two months and is very critical, I have a lot of crashes in production :(
I agree with @luancurti's opinion. This PR has proven to work well in other projects. But, it's not easy to apply the new idea to production service without verification. I think it's a good idea to approach them in stages.
Merged @chakrihacker version of this PR, thanks!
I faced the crash as below.
To solve this problem, we need to check if
ReactContext
is initialized.