Fabric Interop - Properly dispatch integer commands (#38527)

Summary:
Pull Request resolved: #38527

This fixes a bug that got reported for the Fabric Interop for Android with Command dispatching.
(See terrylinla/react-native-sketch-canvas#236)

The problem is that libraries that were receiving commands as ints with:
```
public void receiveCommand(
      int surfaceId, int reactTag, int commandId, Nullable ReadableArray commandArgs) {
```

would not receive command with the Fabric Interop for Android.

The problem is that with Fabric, events are dispatched as string always.
cipolleschi took care of this for iOS, but we realized that the Android part was missing. I'm adding it here.

The logic is, if the event is dispatched as a string that represents a number (say `"42"`) and the user has Fabric Interop enabled, then we dispatch the event as `int` (so libraries will keep on working).

Changelog:
[Android] [Fixed] - Fabric Interop - Properly dispatch integer commands

Reviewed By: cipolleschi

Differential Revision: D47600094

fbshipit-source-id: ee4b4829610c42304bcbf50bd27d2b4093dae760

Author: cortinico

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java

@@ -55,7 +55,6 @@
 import com.facebook.react.common.build.ReactBuildConfig;
 import com.facebook.react.common.mapbuffer.ReadableMapBuffer;
 import com.facebook.react.config.ReactFeatureFlags;
-import com.facebook.react.fabric.events.EventBeatManager;
 import com.facebook.react.fabric.events.EventEmitterWrapper;
 import com.facebook.react.fabric.events.FabricEventEmitter;
 import com.facebook.react.fabric.interfaces.SurfaceHandler;
@@ -65,6 +64,7 @@
 import com.facebook.react.fabric.mounting.SurfaceMountingManager;
 import com.facebook.react.fabric.mounting.SurfaceMountingManager.ViewEvent;
 import com.facebook.react.fabric.mounting.mountitems.BatchMountItem;
+import com.facebook.react.fabric.mounting.mountitems.DispatchCommandMountItem;
 import com.facebook.react.fabric.mounting.mountitems.MountItem;
 import com.facebook.react.fabric.mounting.mountitems.MountItemFactory;
 import com.facebook.react.modules.core.ReactChoreographer;
@@ -79,6 +79,7 @@
 import com.facebook.react.uimanager.UIManagerHelper;
 import com.facebook.react.uimanager.ViewManagerPropertyUpdater;
 import com.facebook.react.uimanager.ViewManagerRegistry;
+import com.facebook.react.uimanager.events.BatchEventDispatchedListener;
 import com.facebook.react.uimanager.events.EventCategoryDef;
 import com.facebook.react.uimanager.events.EventDispatcher;
 import com.facebook.react.uimanager.events.EventDispatcherImpl;
@@ -168,7 +169,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
   @NonNull private final MountItemDispatcher mMountItemDispatcher;
   @NonNull private final ViewManagerRegistry mViewManagerRegistry;
 
-  @NonNull private final EventBeatManager mEventBeatManager;
+  @NonNull private final BatchEventDispatchedListener mBatchEventDispatchedListener;
 
   @NonNull
   private final CopyOnWriteArrayList<UIManagerListener> mListeners = new CopyOnWriteArrayList<>();
@@ -213,14 +214,14 @@ public void executeItems(Queue<MountItem> items) {
   public FabricUIManager(
       @NonNull ReactApplicationContext reactContext,
       @NonNull ViewManagerRegistry viewManagerRegistry,
-      @NonNull EventBeatManager eventBeatManager) {
+      @NonNull BatchEventDispatchedListener batchEventDispatchedListener) {
     mDispatchUIFrameCallback = new DispatchUIFrameCallback(reactContext);
     mReactApplicationContext = reactContext;
     mMountingManager = new MountingManager(viewManagerRegistry, mMountItemExecutor);
     mMountItemDispatcher =
         new MountItemDispatcher(mMountingManager, new MountItemDispatchListener());
     mEventDispatcher = new EventDispatcherImpl(reactContext);
-    mEventBeatManager = eventBeatManager;
+    mBatchEventDispatchedListener = batchEventDispatchedListener;
     mReactApplicationContext.addLifecycleEventListener(this);
 
     mViewManagerRegistry = viewManagerRegistry;
@@ -388,7 +389,7 @@ public void stopSurface(final int surfaceID) {
   @Override
   public void initialize() {
     mEventDispatcher.registerEventEmitter(FABRIC, new FabricEventEmitter(this));
-    mEventDispatcher.addBatchEventDispatchedListener(mEventBeatManager);
+    mEventDispatcher.addBatchEventDispatchedListener(mBatchEventDispatchedListener);
     if (ENABLE_FABRIC_PERF_LOGS) {
       mDevToolsReactPerfLogger = new DevToolsReactPerfLogger();
       mDevToolsReactPerfLogger.addDevToolsReactPerfLoggerListener(FABRIC_PERF_LOGGER);
@@ -427,7 +428,7 @@ public void onCatalystInstanceDestroy() {
     // memory immediately.
     mDispatchUIFrameCallback.stop();
 
-    mEventDispatcher.removeBatchEventDispatchedListener(mEventBeatManager);
+    mEventDispatcher.removeBatchEventDispatchedListener(mBatchEventDispatchedListener);
     mEventDispatcher.unregisterEventEmitter(FABRIC);
 
     mReactApplicationContext.unregisterComponentCallbacks(mViewManagerRegistry);
@@ -1047,9 +1048,17 @@ public void dispatchCommand(
       final int reactTag,
       final String commandId,
       @Nullable final ReadableArray commandArgs) {
-    mMountItemDispatcher.dispatchCommandMountItem(
-        MountItemFactory.createDispatchCommandMountItem(
-            surfaceId, reactTag, commandId, commandArgs));
+    if (ReactFeatureFlags.unstable_useFabricInterop) {
+      // For Fabric Interop, we check if the commandId is an integer. If it is, we use the integer
+      // overload of dispatchCommand. Otherwise, we use the string overload.
+      // and the events won't be correctly dispatched.
+      mMountItemDispatcher.dispatchCommandMountItem(
+          createDispatchCommandMountItemForInterop(surfaceId, reactTag, commandId, commandArgs));
+    } else {
+      mMountItemDispatcher.dispatchCommandMountItem(
+          MountItemFactory.createDispatchCommandMountItem(
+              surfaceId, reactTag, commandId, commandArgs));
+    }
   }
 
   @Override
@@ -1246,6 +1255,26 @@ public void didDispatchMountItems() {
     }
   }
 
+  /**
+   * Util function that takes care of handling commands for Fabric Interop. If the command is a
+   * string that represents a number (say "42"), it will be parsed as an integer and the
+   * corresponding dispatch command mount item will be created.
+   */
+  /* package */ DispatchCommandMountItem createDispatchCommandMountItemForInterop(
+      final int surfaceId,
+      final int reactTag,
+      final String commandId,
+      @Nullable final ReadableArray commandArgs) {
+    try {
+      int commandIdInteger = Integer.parseInt(commandId);
+      return MountItemFactory.createDispatchCommandMountItem(
+          surfaceId, reactTag, commandIdInteger, commandArgs);
+    } catch (NumberFormatException e) {
+      return MountItemFactory.createDispatchCommandMountItem(
+          surfaceId, reactTag, commandId, commandArgs);
+    }
+  }
+
   private class DispatchUIFrameCallback extends GuardedFrameCallback {
 
     private volatile boolean mIsMountingEnabled = true;

packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerTest.kt

@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and 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.fabric
+
+import com.facebook.react.bridge.ReactApplicationContext
+import com.facebook.react.uimanager.ViewManagerRegistry
+import com.facebook.react.uimanager.events.BatchEventDispatchedListener
+import com.facebook.testutils.fakes.FakeBatchEventDispatchedListener
+import com.facebook.testutils.shadows.ShadowSoLoader
+import org.junit.Assert.assertEquals
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.RuntimeEnvironment
+import org.robolectric.annotation.Config
+
+@RunWith(RobolectricTestRunner::class)
+@Config(shadows = [ShadowSoLoader::class])
+class FabricUIManagerTest {
+
+  private lateinit var reactContext: ReactApplicationContext
+  private lateinit var viewManagerRegistry: ViewManagerRegistry
+  private lateinit var batchEventDispatchedListener: BatchEventDispatchedListener
+  private lateinit var underTest: FabricUIManager
+
+  @Before
+  fun setup() {
+    reactContext = ReactApplicationContext(RuntimeEnvironment.getApplication())
+    viewManagerRegistry = ViewManagerRegistry(emptyList())
+    batchEventDispatchedListener = FakeBatchEventDispatchedListener()
+    underTest = FabricUIManager(reactContext, viewManagerRegistry, batchEventDispatchedListener)
+  }
+
+  @Test
+  fun createDispatchCommandMountItemForInterop_withValidString_returnsStringEvent() {
+    val command = underTest.createDispatchCommandMountItemForInterop(11, 1, "anEvent", null)
+
+    // DispatchStringCommandMountItem is package private so we can `as` check it.
+    val className = command::class.java.name.substringAfterLast(".")
+    assertEquals("DispatchStringCommandMountItem", className)
+  }
+
+  @Test
+  fun createDispatchCommandMountItemForInterop_withValidInt_returnsIntEvent() {
+    val command = underTest.createDispatchCommandMountItemForInterop(11, 1, "42", null)
+
+    // DispatchIntCommandMountItem is package private so we can `as` check it.
+    val className = command::class.java.name.substringAfterLast(".")
+    assertEquals("DispatchIntCommandMountItem", className)
+  }
+}

packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/fakes/FakeBatchEventDispatchedListener.kt

@@ -0,0 +1,17 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and 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.testutils.fakes
+
+import com.facebook.react.uimanager.events.BatchEventDispatchedListener
+
+/** A fake [BatchEventDispatchedListener] for testing that does nothing. */
+class FakeBatchEventDispatchedListener : BatchEventDispatchedListener {
+  override fun onBatchEventDispatched() {
+    // do nothing
+  }
+}

packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/component/MyLegacyViewManager.java

@@ -27,7 +27,7 @@
 public class MyLegacyViewManager extends SimpleViewManager<MyNativeView> {
 
   public static final String REACT_CLASS = "RNTMyLegacyNativeView";
-  private static final Integer COMMAND_CHANGE_BACKGROUND_COLOR = 42;
+  private static final int COMMAND_CHANGE_BACKGROUND_COLOR = 42;
   private final ReactApplicationContext mCallerContext;
 
   public MyLegacyViewManager(ReactApplicationContext reactContext) {
@@ -71,8 +71,7 @@ public final Map<String, Object> getExportedViewConstants() {
 
   @Override
   public final Map<String, Object> getExportedCustomBubblingEventTypeConstants() {
-    Map<String, Object> eventTypeConstants = new HashMap<String, Object>();
-    eventTypeConstants.putAll(
+    return new HashMap<>(
         MapBuilder.<String, Object>builder()
             .put(
                 "onColorChanged",
@@ -81,18 +80,29 @@ public final Map<String, Object> getExportedCustomBubblingEventTypeConstants() {
                     MapBuilder.of(
                         "bubbled", "onColorChanged", "captured", "onColorChangedCapture")))
             .build());
-    return eventTypeConstants;
   }
 
   @Override
   public void receiveCommand(
       @NonNull MyNativeView view, String commandId, @Nullable ReadableArray args) {
-    if (commandId.contentEquals(COMMAND_CHANGE_BACKGROUND_COLOR.toString())) {
+    if (commandId.contentEquals("changeBackgroundColor")) {
       int sentColor = Color.parseColor(args.getString(0));
       view.setBackgroundColor(sentColor);
     }
   }
 
+  @SuppressWarnings("deprecation") // We intentionally want to test against the legacy API here.
+  @Override
+  public void receiveCommand(
+      @NonNull MyNativeView view, int commandId, @Nullable ReadableArray args) {
+    switch (commandId) {
+      case COMMAND_CHANGE_BACKGROUND_COLOR:
+        int sentColor = Color.parseColor(args.getString(0));
+        view.setBackgroundColor(sentColor);
+        break;
+    }
+  }
+
   @Nullable
   @Override
   public Map<String, Integer> getCommandsMap() {