From 0a12f3ea77840c218fd9d67d0e3f2ea068cb8c5f Mon Sep 17 00:00:00 2001 From: Emily Janzer Date: Wed, 27 May 2020 12:10:14 -0700 Subject: [PATCH] Always return an EventDispatcher in bridgeless mode Summary: This changes how we access the EventDispatcher from the FabricUIManager in bridgeless mode. Currently, we have implemented a similar API to what we use for Fabric (used in UIManagerHelper): `BridgelessReactContext.getJSIModule(UIManager).getEventDispatcher()`. However, `getJSIModule` does not have a nullable return type, which means that we have to throw an exception if the UIManager can't be found - if, for example, the instance is not initialized yet (or has been destroyed). This is causing crashes when a view tries to access the EventDispatcher before the instance is initialized, which takes longer for Venice because we include JS bundle loading as part of initialization (we may need to revisit that). Ideally, we'd like to create a first-class API for `getEventDispatcher()`, and make sure that it never crashes if the instance is destroyed, because we don't care if JS events aren't delivered at that point. However, there are some obstacles to making this change for the bridge - mostly related to avoiding circular dependencies between the bridge module and the uimanager module. (Also, this might be a behavior change for the bridge, because I think we currently start queueing events before it's initialized? and product code might be relying on that). Reviewed By: mdvacca Differential Revision: D21672949 fbshipit-source-id: a38e96cd40c6f70124b7ca2a5c9722988fe7fcf4 --- .../react/uimanager/ThemedReactContext.java | 14 ++--- .../react/uimanager/UIManagerHelper.java | 50 ++++++++-------- .../events/BlackHoleEventDispatcher.java | 57 +++++++++++++++++++ .../events/EventDispatcherProvider.java | 24 ++++++++ 4 files changed, 112 insertions(+), 33 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/uimanager/events/BlackHoleEventDispatcher.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/uimanager/events/EventDispatcherProvider.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ThemedReactContext.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ThemedReactContext.java index 4eff054efe9..039798dcaf3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ThemedReactContext.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ThemedReactContext.java @@ -10,8 +10,6 @@ package com.facebook.react.uimanager; import android.app.Activity; import android.content.Context; import androidx.annotation.Nullable; -import com.facebook.react.bridge.JSIModule; -import com.facebook.react.bridge.JSIModuleType; import com.facebook.react.bridge.LifecycleEventListener; import com.facebook.react.bridge.ReactApplicationContext; import com.facebook.react.bridge.ReactContext; @@ -71,16 +69,12 @@ public class ThemedReactContext extends ReactContext { return mSurfaceID; } + public ReactApplicationContext getReactApplicationContext() { + return mReactApplicationContext; + } + @Override public boolean isBridgeless() { return mReactApplicationContext.isBridgeless(); } - - @Override - public JSIModule getJSIModule(JSIModuleType moduleType) { - if (isBridgeless()) { - return mReactApplicationContext.getJSIModule(moduleType); - } - return super.getJSIModule(moduleType); - } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerHelper.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerHelper.java index c034e06830b..bd978336c2d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerHelper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerHelper.java @@ -22,6 +22,7 @@ import com.facebook.react.bridge.ReactSoftException; import com.facebook.react.bridge.UIManager; import com.facebook.react.uimanager.common.UIManagerType; import com.facebook.react.uimanager.events.EventDispatcher; +import com.facebook.react.uimanager.events.EventDispatcherProvider; /** Helper class for {@link UIManager}. */ public class UIManagerHelper { @@ -43,32 +44,28 @@ public class UIManagerHelper { ReactContext context, @UIManagerType int uiManagerType, boolean returnNullIfCatalystIsInactive) { - if (context.isBridgeless()) { - return (UIManager) context.getJSIModule(JSIModuleType.UIManager); - } else { - if (!context.hasCatalystInstance()) { - ReactSoftException.logSoftException( - "UIManagerHelper", - new ReactNoCrashSoftException( - "Cannot get UIManager because the context doesn't contain a CatalystInstance.")); + if (!context.hasCatalystInstance()) { + ReactSoftException.logSoftException( + "UIManagerHelper", + new ReactNoCrashSoftException( + "Cannot get UIManager because the context doesn't contain a CatalystInstance.")); + return null; + } + // TODO T60461551: add tests to verify emission of events when the ReactContext is being turn + // down. + if (!context.hasActiveCatalystInstance()) { + ReactSoftException.logSoftException( + "UIManagerHelper", + new ReactNoCrashSoftException( + "Cannot get UIManager because the context doesn't contain an active CatalystInstance.")); + if (returnNullIfCatalystIsInactive) { return null; } - // TODO T60461551: add tests to verify emission of events when the ReactContext is being turn - // down. - if (!context.hasActiveCatalystInstance()) { - ReactSoftException.logSoftException( - "UIManagerHelper", - new ReactNoCrashSoftException( - "Cannot get UIManager because the context doesn't contain an active CatalystInstance.")); - if (returnNullIfCatalystIsInactive) { - return null; - } - } - CatalystInstance catalystInstance = context.getCatalystInstance(); - return uiManagerType == FABRIC - ? (UIManager) catalystInstance.getJSIModule(JSIModuleType.UIManager) - : catalystInstance.getNativeModule(UIManagerModule.class); } + CatalystInstance catalystInstance = context.getCatalystInstance(); + return uiManagerType == FABRIC + ? (UIManager) catalystInstance.getJSIModule(JSIModuleType.UIManager) + : catalystInstance.getNativeModule(UIManagerModule.class); } /** @@ -87,6 +84,13 @@ public class UIManagerHelper { @Nullable public static EventDispatcher getEventDispatcher( ReactContext context, @UIManagerType int uiManagerType) { + // TODO T67518514 Clean this up once we migrate everything over to bridgeless mode + if (context.isBridgeless()) { + if (context instanceof ThemedReactContext) { + context = ((ThemedReactContext) context).getReactApplicationContext(); + } + return ((EventDispatcherProvider) context).getEventDispatcher(); + } UIManager uiManager = getUIManager(context, uiManagerType, false); return uiManager == null ? null : (EventDispatcher) uiManager.getEventDispatcher(); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/BlackHoleEventDispatcher.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/BlackHoleEventDispatcher.java new file mode 100644 index 00000000000..1fce7974087 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/BlackHoleEventDispatcher.java @@ -0,0 +1,57 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.uimanager.events; + +import com.facebook.common.logging.FLog; + +/** + * A singleton class that overrides {@link EventDispatcher} with no-op methods, to be used by + * callers that expect an EventDispatcher when the instance doesn't exist. + */ +public class BlackHoleEventDispatcher implements EventDispatcher { + + private static final EventDispatcher sEventDispatcher = new BlackHoleEventDispatcher(); + + public static EventDispatcher get() { + return sEventDispatcher; + } + + private BlackHoleEventDispatcher() {} + + @Override + public void dispatchEvent(Event event) { + FLog.d( + getClass().getSimpleName(), + "Trying to emit event to JS, but the React instance isn't ready. Event: " + + event.getEventName()); + } + + @Override + public void dispatchAllEvents() {} + + @Override + public void addListener(EventDispatcherListener listener) {} + + @Override + public void removeListener(EventDispatcherListener listener) {} + + @Override + public void addBatchEventDispatchedListener(BatchEventDispatchedListener listener) {} + + @Override + public void removeBatchEventDispatchedListener(BatchEventDispatchedListener listener) {} + + @Override + public void registerEventEmitter(int uiManagerType, RCTEventEmitter eventEmitter) {} + + @Override + public void unregisterEventEmitter(int uiManagerType) {} + + @Override + public void onCatalystInstanceDestroyed() {} +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/EventDispatcherProvider.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/EventDispatcherProvider.java new file mode 100644 index 00000000000..6f6537c068c --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/EventDispatcherProvider.java @@ -0,0 +1,24 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.uimanager.events; + +/** + * An interface that can be implemented by a {@link com.facebook.react.bridge.ReactContext} to + * provide a first-class API for accessing the {@link EventDispatcher} from the {@link + * com.facebook.react.bridge.UIManager}. + */ +public interface EventDispatcherProvider { + + /** + * This method should always return an EventDispatcher, even if the instance doesn't exist; in + * that case it should return the empty {@link BlackHoleEventDispatcher}. + * + * @return An {@link EventDispatcher} to emit events to JS. + */ + EventDispatcher getEventDispatcher(); +}