-
Notifications
You must be signed in to change notification settings - Fork 238
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(android): forward-port getReactContext to new arch bridgeless mode #1127
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1127 +/- ##
=======================================
Coverage 77.08% 77.08%
=======================================
Files 32 32
Lines 1727 1727
Branches 579 556 -23
=======================================
Hits 1331 1331
- Misses 343 395 +52
+ Partials 53 1 -52 |
...but do so in a way that is backwards-compatible as the symbols required - do not show up at all until react-native 0.73 - moved to different package names in 0.74
4c5afba
to
64dc8fa
Compare
Here is the patch if anyone wants to test it: https://github.com/mikehardy/rnfbdemo/blob/rn76/patches/%40notifee%2Breact-native%2B9.1.1.patch diff --git a/node_modules/@notifee/react-native/android/src/main/java/io/invertase/notifee/NotifeeReactUtils.java b/node_modules/@notifee/react-native/android/src/main/java/io/invertase/notifee/NotifeeReactUtils.java
index 4c3b169..549bcf0 100644
--- a/node_modules/@notifee/react-native/android/src/main/java/io/invertase/notifee/NotifeeReactUtils.java
+++ b/node_modules/@notifee/react-native/android/src/main/java/io/invertase/notifee/NotifeeReactUtils.java
@@ -12,6 +12,8 @@ import android.os.Handler;
import android.os.Looper;
import android.util.Log;
import android.util.SparseArray;
+
+import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.lifecycle.Lifecycle;
import androidx.lifecycle.ProcessLifecycleOwner;
@@ -105,11 +107,41 @@ class NotifeeReactUtils {
}
}
+ @SuppressLint("VisibleForTests")
private static @Nullable ReactContext getReactContext() {
- ReactNativeHost reactNativeHost =
+ try {
+
+ // Carefully try to load new architecture classes so we preserve backwards compatibility
+ // These symbols are only available in react-native >= 0.73
+ try {
+ Class<?> entryPoint = Class.forName("com.facebook.react.defaults.DefaultNewArchitectureEntryPoint");
+ Method bridgelessEnabled = entryPoint.getMethod("getBridgelessEnabled");
+ Object result = bridgelessEnabled.invoke(null);
+ if (result == Boolean.TRUE) {
+ Log.d("getReactContext", "We are in bridgeless new architecture mode");
+ Object reactApplication = EventSubscriber.getContext();
+ Method getReactHost = reactApplication.getClass().getMethod("getReactHost");
+ Object reactHostInstance = getReactHost.invoke(reactApplication);
+ Method getCurrentReactContext = reactHostInstance.getClass().getMethod("getCurrentReactContext");
+ return (ReactContext)getCurrentReactContext.invoke(reactHostInstance);
+ } else {
+ Log.d("getReactContext", "we are NOT in bridgeless new architecture mode");
+ }
+ } catch (Exception e) {
+ Log.d("getReactContext", "New Architecture class load failed. Using fallback.");
+ }
+
+ Log.d("getReactContext", "Determining ReactContext using fallback method");
+ ReactNativeHost reactNativeHost =
((ReactApplication) EventSubscriber.getContext()).getReactNativeHost();
- ReactInstanceManager reactInstanceManager = reactNativeHost.getReactInstanceManager();
- return reactInstanceManager.getCurrentReactContext();
+ ReactInstanceManager reactInstanceManager = reactNativeHost.getReactInstanceManager();
+ return reactInstanceManager.getCurrentReactContext();
+ } catch (Exception e) {
+ Log.w("getReactContext", "ReactHost unexpectedly null", e);
+ }
+
+ Log.w("getReactContext", "Unable to determine ReactContext");
+ return null;
}
private static void initializeReactContext(GenericCallback callback) {
@@ -121,7 +153,7 @@ class NotifeeReactUtils {
reactInstanceManager.addReactInstanceEventListener(
new ReactInstanceManager.ReactInstanceEventListener() {
@Override
- public void onReactContextInitialized(final ReactContext reactContext) {
+ public void onReactContextInitialized(@NonNull final ReactContext reactContext) {
reactInstanceManager.removeReactInstanceEventListener(this);
new Handler(Looper.getMainLooper()).postDelayed(callback::call, 100);
}
|
This PR did not affect the iOS code thus that CI failure is a flake (and indeed, there is some issue with the simulator even starting the app). Not blocking |
This is released as 9.1.2 |
...but do so in a way that is backwards-compatible as the symbols required
Test plan:
I used this script, on my react-native 0.76 testing branch, to verify that it worked:
https://github.com/mikehardy/rnfbdemo/blob/rn76/notifee-demo.sh
I verified it down to react-native 0.72 by changing this line to the various stable versions (0.75.4, 0.74.6, 0.73.10, 0.72.17):
https://github.com/mikehardy/rnfbdemo/blob/1064dcd31a031e9abb53f2611bf32fda5470ee28/notifee-demo.sh#L4
Of particular note is that it contains the patch which I propose here, and is what made things work, with a huge debt of gratitude to @Eclipses-Saros for the initial investigation, idea, and testing. My contribution was making it reflective for backwards compatibility then back-testing it from 0.76.0-rc.6 down to 0.72.17
The test is simply to build the app on android, display a notification, and interact with the notification to make sure events are registered.
The notifee-demo.sh script installs an App that does that - it posts notification and logs the events
Before this patch, the events stopped working with either new architecture enabled on older react-native versions, or with react-native 0.76 in the default configuration (which is new arch enabled + bridgeless)
With the patch, things work everywhere all the time.
Fixes #621
Related #1077