Make sure callbacks are not reused and reloading works

Summary:
This diff is part of the bigger task T60496135

This diff changes the RN support from crude to decent citizen, making sure we don't recycle callbacks over the bridge, use subscriptions were possible, and making sure connecting, disconnecting, etc works correctly

For example, connect and disconnect hooks should work.

Finally, throw in hot reloading into the mix, which causes the registerPlugin to be triggered another time, without the old one every been unloaded.
This should trigger a new 'onConnect' on the client, to make sure it can restore any state / subscriptions necessary, even though the never disappeared in the Java world.

These cases should all be handled well.

Reviewed By: jknoxville

Differential Revision: D19347330

fbshipit-source-id: de64a08f4043f01528c794430ccc3c717abf0180

react-native/react-native-flipper/android/src/main/java/com/facebook/flipper/reactnative/FlipperModule.java

@@ -7,44 +7,31 @@
 
 package com.facebook.flipper.reactnative;
 
-import com.facebook.flipper.android.AndroidFlipperClient;
-import com.facebook.flipper.core.FlipperArray;
-import com.facebook.flipper.core.FlipperClient;
-import com.facebook.flipper.core.FlipperConnection;
-import com.facebook.flipper.core.FlipperObject;
-import com.facebook.flipper.core.FlipperPlugin;
-import com.facebook.flipper.core.FlipperReceiver;
-import com.facebook.flipper.core.FlipperResponder;
-import com.facebook.react.bridge.Callback;
 import com.facebook.react.bridge.ReactApplicationContext;
 import com.facebook.react.bridge.ReactContextBaseJavaModule;
 import com.facebook.react.bridge.ReactMethod;
+import com.facebook.react.bridge.WritableMap;
 import com.facebook.react.module.annotations.ReactModule;
-import java.util.Map;
+import com.facebook.react.modules.core.DeviceEventManagerModule;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.atomic.AtomicLong;
-import org.json.JSONArray;
-import org.json.JSONException;
-import org.json.JSONObject;
-import org.json.JSONTokener;
 
+/**
+ * The FlipperModule is a React Native Native Module. The instance handles incoming calls that
+ * arrive over the React Native bridge from JavaScript. The instance is created per
+ * ReactApplicationContext. Which means this module gets reinstated if RN performs a reload. So it
+ * should not hold any further state on its own. All state is hold by the Plugin and PluginManager
+ * classes.
+ */
 @ReactModule(name = FlipperModule.NAME)
 public class FlipperModule extends ReactContextBaseJavaModule {
 
   public static final String NAME = "Flipper";
 
-  private final ReactApplicationContext reactContext;
+  private FlipperReactNativeJavaScriptPluginManager manager;
-  private final FlipperClient flipperClient;
-  private final Map<String, FlipperConnection> connections;
-  private final Map<String, FlipperResponder> responders;
-  private final AtomicLong responderId = new AtomicLong();
 
-  public FlipperModule(ReactApplicationContext reactContext) {
+  public FlipperModule(
+      FlipperReactNativeJavaScriptPluginManager manager, ReactApplicationContext reactContext) {
     super(reactContext);
-    this.reactContext = reactContext;
+    this.manager = manager;
-    this.flipperClient = AndroidFlipperClient.getInstanceIfInitialized();
-    this.connections = new ConcurrentHashMap<>();
-    this.responders = new ConcurrentHashMap<>();
   }
 
   @Override
@@ -53,118 +40,46 @@ public class FlipperModule extends ReactContextBaseJavaModule {
   }
 
   @ReactMethod
-  public void registerPlugin(
+  public void registerPlugin(final String pluginId, final Boolean inBackground) {
-      final String pluginId,
+    this.manager.registerPlugin(this, pluginId, inBackground);
-      final Boolean inBackground,
-      final Callback onConnect,
-      final Callback onDisconnect) {
-    FlipperPlugin plugin =
-        new FlipperPlugin() {
-          @Override
-          public String getId() {
-            return pluginId;
-          }
-
-          @Override
-          public void onConnect(FlipperConnection connection) throws Exception {
-            FlipperModule.this.connections.put(pluginId, connection);
-            onConnect.invoke();
-          }
-
-          @Override
-          public void onDisconnect() throws Exception {
-            FlipperModule.this.connections.remove(pluginId);
-            onDisconnect.invoke();
-          }
-
-          @Override
-          public boolean runInBackground() {
-            return inBackground;
-          }
-        };
-    this.flipperClient.addPlugin(plugin);
   }
 
   @ReactMethod
   public void send(String pluginId, String method, String data) {
-    // Optimization: throwing raw strings around to the desktop would probably avoid some double
+    this.manager.send(pluginId, method, data);
-    // parsing...
-    Object parsedData = FlipperModule.parseJSON(data);
-    FlipperConnection connection = this.connections.get(pluginId);
-    if (parsedData instanceof FlipperArray) {
-      connection.send(method, (FlipperArray) parsedData);
-    } else {
-      connection.send(method, (FlipperObject) parsedData);
-    }
   }
 
   @ReactMethod
   public void reportErrorWithMetadata(String pluginId, String reason, String stackTrace) {
-    this.connections.get(pluginId).reportErrorWithMetadata(reason, stackTrace);
+    this.manager.reportErrorWithMetadata(pluginId, reason, stackTrace);
   }
 
   @ReactMethod
   public void reportError(String pluginId, String error) {
-    this.connections.get(pluginId).reportError(new Error(error));
+    this.manager.reportError(pluginId, error);
   }
 
   @ReactMethod
-  public void subscribe(String pluginId, String method, final Callback callback) {
+  public void subscribe(String pluginId, String method) {
-    this.connections
+    this.manager.subscribe(this, pluginId, method);
-        .get(pluginId)
-        .receive(
-            method,
-            new FlipperReceiver() {
-
-              @Override
-              public void onReceive(FlipperObject params, FlipperResponder responder)
-                  throws Exception {
-                String id = String.valueOf(FlipperModule.this.responderId.incrementAndGet());
-                FlipperModule.this.responders.put(id, responder);
-                callback.invoke(params.toJsonString(), id);
-              }
-            });
   }
 
   @ReactMethod
   public void respondSuccess(String responderId, String data) {
-    FlipperResponder responder = FlipperModule.this.responders.remove(responderId);
+    this.manager.respondSuccess(responderId, data);
-    if (data == null) {
-      responder.success();
-    } else {
-      Object parsedData = FlipperModule.parseJSON(data);
-      if (parsedData instanceof FlipperArray) {
-        responder.success((FlipperArray) parsedData);
-      } else {
-        responder.success((FlipperObject) parsedData);
-      }
-    }
   }
 
   @ReactMethod
   public void respondError(String responderId, String data) {
-    FlipperResponder responder = FlipperModule.this.responders.remove(responderId);
+    this.manager.respondError(responderId, data);
-    Object parsedData = FlipperModule.parseJSON(data);
-    if (parsedData instanceof FlipperArray) {
-      responder.success((FlipperArray) parsedData);
-    } else {
-      responder.success((FlipperObject) parsedData);
-    }
   }
 
-  private static Object /* FlipperArray | FlipperObject */ parseJSON(String json) {
+  public void sendJSEvent(String eventName, WritableMap params) {
-    // returns either a FlipperObject or Flipper array, pending the data
+    ReactApplicationContext context = getReactApplicationContextIfActiveOrWarn();
-    try {
+    if (context != null) {
-      JSONTokener tokener = new JSONTokener(json);
+      context
-      if (tokener.nextClean() == '[') {
+          .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
-        tokener.back();
+          .emit(eventName, params);
-        return new FlipperArray(new JSONArray(tokener));
-      } else {
-        tokener.back();
-        return new FlipperObject(new JSONObject(tokener));
-      }
-    } catch (JSONException e) {
-      throw new RuntimeException(e);
     }
   }
 }

react-native/react-native-flipper/android/src/main/java/com/facebook/flipper/reactnative/FlipperPackage.java

@@ -15,10 +15,17 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
+/**
+ * Exposes the react native modules that should be created per ReactApplicationContext. Note that an
+ * application context lives shorter than the application itself, e.g. reload creates a fresh one.
+ */
 public class FlipperPackage implements ReactPackage {
+  static FlipperModule flipperModule;
+
   @Override
   public List<NativeModule> createNativeModules(ReactApplicationContext reactContext) {
-    return Arrays.<NativeModule>asList(new FlipperModule(reactContext));
+    return Arrays.<NativeModule>asList(
+        new FlipperModule(FlipperReactNativeJavaScriptPluginManager.getInstance(), reactContext));
   }
 
   @Override

react-native/react-native-flipper/android/src/main/java/com/facebook/flipper/reactnative/FlipperReactNativeJavaScriptPlugin.java

@@ -0,0 +1,88 @@
+/*
+ * 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.flipper.reactnative;
+
+import com.facebook.flipper.core.FlipperConnection;
+import com.facebook.flipper.core.FlipperPlugin;
+import com.facebook.react.bridge.Arguments;
+import com.facebook.react.bridge.WritableMap;
+
+/**
+ * This class holds the state of a single plugin that is created from the JS world trough
+ * `Flipper.addPlugin`. It's main concern is managing the FlipperConnection to the Desktop client.
+ *
+ * <p>This class is abstract, as Flipper does not support having multiple instances of the same
+ * class as plugins, But every JS plugin will store it state in a FlipperPlugin, so for every plugin
+ * we will create an anonymous subclass.
+ *
+ * <p>Note that this class does not directly interact back over the JS bridge to React Native, as
+ * the JavaPlugin has a longer lifecycle than it's JS counter part, which will be recreated on
+ * reload. However, if the native module reload, we keep these instances to not loose the connextion
+ * to the Flipper Desktop client.
+ */
+public abstract class FlipperReactNativeJavaScriptPlugin implements FlipperPlugin {
+  String pluginId;
+  boolean inBackground;
+  FlipperConnection connection;
+  FlipperModule module;
+
+  public FlipperReactNativeJavaScriptPlugin(
+      FlipperModule module, String pluginId, boolean inBackground) {
+    this.pluginId = pluginId;
+    this.module = module;
+    this.inBackground = inBackground;
+    this.module = module;
+  }
+
+  @Override
+  public String getId() {
+    return pluginId;
+  }
+
+  @Override
+  public void onConnect(FlipperConnection connection) throws Exception {
+    this.connection = connection;
+    this.fireOnConnect();
+  }
+
+  public void fireOnConnect() {
+    if (!isConnected()) {
+      throw new RuntimeException("Plugin not connected " + pluginId);
+    }
+    this.module.sendJSEvent("react-native-flipper-plugin-connect", getPluginParams());
+  }
+
+  @Override
+  public void onDisconnect() throws Exception {
+    this.module.sendJSEvent("react-native-flipper-plugin-disconnect", getPluginParams());
+    this.connection = null;
+  }
+
+  @Override
+  public boolean runInBackground() {
+    return inBackground;
+  }
+
+  public boolean isConnected() {
+    return connection != null;
+  }
+
+  public FlipperConnection getConnection() {
+    return connection;
+  }
+
+  public void setModule(FlipperModule module) {
+    this.module = module;
+  }
+
+  WritableMap getPluginParams() {
+    WritableMap params = Arguments.createMap();
+    params.putString("plugin", pluginId);
+    return params;
+  }
+}

react-native/react-native-flipper/android/src/main/java/com/facebook/flipper/reactnative/FlipperReactNativeJavaScriptPluginManager.java

@@ -0,0 +1,151 @@
+/*
+ * 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.flipper.reactnative;
+
+import com.facebook.flipper.android.AndroidFlipperClient;
+import com.facebook.flipper.core.FlipperArray;
+import com.facebook.flipper.core.FlipperClient;
+import com.facebook.flipper.core.FlipperObject;
+import com.facebook.flipper.core.FlipperResponder;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.json.JSONTokener;
+
+/**
+ * This class manages all loaded FlipperPlugins. It is a singleton to make sure plugin states are
+ * preserved even when the native modules get reloaded (e.g. due to a reload in RN). This avoids
+ * loosing our connections with Flipper.
+ *
+ * <p>Note that this manager is not bound to a specific FlipperModule instance, as that might be
+ * swapped in and out over time.
+ */
+public class FlipperReactNativeJavaScriptPluginManager {
+  static FlipperReactNativeJavaScriptPluginManager instance;
+
+  static FlipperReactNativeJavaScriptPluginManager getInstance() {
+    if (instance == null) {
+      instance = new FlipperReactNativeJavaScriptPluginManager();
+    }
+    return instance;
+  }
+
+  private final FlipperClient flipperClient;
+
+  // uniqueResponderId -> ResponderObject
+  private final Map<String, FlipperResponder> responders = new ConcurrentHashMap<>();
+  // generated the next responder id
+  private final AtomicLong responderId = new AtomicLong();
+
+  private FlipperReactNativeJavaScriptPluginManager() {
+    instance = this;
+    this.flipperClient = AndroidFlipperClient.getInstanceIfInitialized();
+  }
+
+  public void registerPlugin(
+      FlipperModule module, final String pluginId, final Boolean inBackground) {
+    FlipperReactNativeJavaScriptPlugin existing = getPlugin(pluginId);
+    if (existing != null) {
+      // Make sure events are emitted on the right application context
+      existing.setModule(module);
+      // this happens if the plugin hot reloaded on JS side, but we had it here already
+      if (existing.isConnected()) {
+        existing.fireOnConnect();
+      }
+      return;
+    }
+    // we always create a new plugin class on the fly,
+    // as Flipper only allows one plugin per type to be registered!
+    FlipperReactNativeJavaScriptPlugin plugin =
+        new FlipperReactNativeJavaScriptPlugin(module, pluginId, inBackground) {
+          // inner class with no new members
+        };
+    this.flipperClient.addPlugin(plugin);
+  }
+
+  public void send(String pluginId, String method, String data) {
+    // Optimization: throwing raw strings around to the desktop would probably avoid some double
+    // parsing...
+    Object parsedData = parseJSON(data);
+    FlipperReactNativeJavaScriptPlugin plugin = getPlugin(pluginId);
+    if (parsedData instanceof FlipperArray) {
+      plugin.getConnection().send(method, (FlipperArray) parsedData);
+    } else {
+      plugin.getConnection().send(method, (FlipperObject) parsedData);
+    }
+  }
+
+  public void reportErrorWithMetadata(String pluginId, String reason, String stackTrace) {
+    getPlugin(pluginId).getConnection().reportErrorWithMetadata(reason, stackTrace);
+  }
+
+  public void reportError(String pluginId, String error) {
+    getPlugin(pluginId).getConnection().reportError(new Error(error));
+  }
+
+  public void subscribe(FlipperModule module, String pluginId, String method) {
+    String key = pluginId + "#" + method;
+    FlipperReactNativeJavaScriptReceiver receiver =
+        new FlipperReactNativeJavaScriptReceiver(this, module, pluginId, method);
+    // Fresh connection should be the case for a new subscribe...
+    getPlugin(pluginId).getConnection().receive(method, receiver);
+  }
+
+  public void respondSuccess(String responderId, String data) {
+    FlipperResponder responder = responders.remove(responderId);
+    if (data == null) {
+      responder.success();
+    } else {
+      Object parsedData = parseJSON(data);
+      if (parsedData instanceof FlipperArray) {
+        responder.success((FlipperArray) parsedData);
+      } else {
+        responder.success((FlipperObject) parsedData);
+      }
+    }
+  }
+
+  public void respondError(String responderId, String data) {
+    FlipperResponder responder = responders.remove(responderId);
+    Object parsedData = parseJSON(data);
+    if (parsedData instanceof FlipperArray) {
+      responder.success((FlipperArray) parsedData);
+    } else {
+      responder.success((FlipperObject) parsedData);
+    }
+  }
+
+  FlipperReactNativeJavaScriptPlugin getPlugin(String pluginId) {
+    return this.flipperClient.<FlipperReactNativeJavaScriptPlugin>getPlugin(pluginId);
+  }
+
+  public String createResponderId(FlipperResponder responder) {
+    String id = String.valueOf(responderId.incrementAndGet());
+    responders.put(id, responder);
+    return id;
+  }
+
+  private static Object /* FlipperArray | FlipperObject */ parseJSON(String json) {
+    // returns either a FlipperObject or Flipper array, pending the data
+    try {
+      JSONTokener tokener = new JSONTokener(json);
+      if (tokener.nextClean() == '[') {
+        tokener.back();
+        return new FlipperArray(new JSONArray(tokener));
+      } else {
+        tokener.back();
+        return new FlipperObject(new JSONObject(tokener));
+      }
+    } catch (JSONException e) {
+      throw new RuntimeException(e);
+    }
+  }
+}

react-native/react-native-flipper/android/src/main/java/com/facebook/flipper/reactnative/FlipperReactNativeJavaScriptReceiver.java

@@ -0,0 +1,43 @@
+/*
+ * 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.flipper.reactnative;
+
+import com.facebook.flipper.core.FlipperObject;
+import com.facebook.flipper.core.FlipperReceiver;
+import com.facebook.flipper.core.FlipperResponder;
+import com.facebook.react.bridge.Arguments;
+import com.facebook.react.bridge.WritableMap;
+
+public class FlipperReactNativeJavaScriptReceiver implements FlipperReceiver {
+  String plugin;
+  String method;
+  FlipperReactNativeJavaScriptPluginManager manager;
+  FlipperModule module;
+
+  public FlipperReactNativeJavaScriptReceiver(
+      FlipperReactNativeJavaScriptPluginManager manager,
+      FlipperModule module,
+      String plugin,
+      String method) {
+    this.plugin = plugin;
+    this.method = method;
+    this.manager = manager;
+    this.module = module;
+  }
+
+  @Override
+  public void onReceive(FlipperObject params, FlipperResponder responder) throws Exception {
+    String responderId = manager.createResponderId(responder);
+    WritableMap eventData = Arguments.createMap();
+    eventData.putString("plugin", plugin);
+    eventData.putString("method", method);
+    eventData.putString("params", params.toJsonString());
+    eventData.putString("responderId", responderId);
+    module.sendJSEvent("react-native-flipper-receive-event", eventData);
+  }
+}

react-native/react-native-flipper/index.d.ts

@@ -57,12 +57,13 @@ declare namespace Flipper {
   }
 }
 
+/**
+ * Internal api to connect to the native Java module, not to be used directly
+ */
 declare module 'Flipper' {
   export function registerPlugin(
     pluginId: string,
     runInBackground: boolean,
-    onConnect: () => void,
-    onDisconnect: () => void,
   ): void;
   export function send(pluginId: string, method: string, data: string): void;
   export function reportErrorWithMetadata(
@@ -71,13 +72,13 @@ declare module 'Flipper' {
     stackTrace: string,
   ): void;
   export function reportError(pluginId: string, error: string): void;
-  export function subscribe(
+  export function subscribe(pluginId: string, method: string): void;
-    pluginId: string,
-    method: string,
-    listener: (data: string, responderId: number) => void,
-  ): void;
   export function respondSuccess(responderId: string, data?: string): void;
   export function respondError(responderId: string, error: string): void;
 }
 
+/**
+ * Register a new plugin
+ * @param plugin
+ */
 export function addPlugin(plugin: Flipper.FlipperPlugin): void;

react-native/react-native-flipper/index.js

@@ -8,12 +8,15 @@
  */
 
 // $FlowFixMe
-import {NativeModules} from 'react-native';
+import {NativeModules, NativeEventEmitter} from 'react-native';
 
 const {Flipper} = NativeModules;
 
 export default Flipper;
 
+const listeners = {}; // plugin#method -> callback
+const plugins = {}; // plugin -> Plugin
+
 class Connection {
   connected;
   pluginId;
@@ -42,10 +45,9 @@ class Connection {
     if (!this.connected) {
       throw new Error('Cannot receive data, not connected');
     }
-    Flipper.subscribe(this.pluginId, method, (data, responderId) => {
+
-      const responder = new Responder(responderId);
+    listeners[this.pluginId + '#' + method] = listener;
-      listener(JSON.parse(data), responder);
+    Flipper.subscribe(this.pluginId, method);
-    });
   }
 }
 
@@ -68,6 +70,40 @@ class Responder {
   }
 }
 
+function startEventListeners() {
+  const emitter = new NativeEventEmitter(Flipper);
+  emitter.removeAllListeners('react-native-flipper-plugin-connect');
+  emitter.removeAllListeners('react-native-flipper-plugin-disconnect');
+  emitter.removeAllListeners('react-native-flipper-receive-event');
+
+  emitter.addListener('react-native-flipper-plugin-connect', event => {
+    const {plugin} = event;
+    if (plugins[plugin]) {
+      const p = plugins[plugin];
+      p._connection.connected = true;
+      p.onConnect(p._connection);
+    }
+  });
+
+  emitter.addListener('react-native-flipper-plugin-disconnect', event => {
+    const {plugin} = event;
+    if (plugins[plugin]) {
+      const p = plugins[plugin];
+      p._connection.connected = false;
+      p.onDisconnect();
+    }
+  });
+
+  emitter.addListener('react-native-flipper-receive-event', event => {
+    const {plugin, method, params, responderId} = event;
+    const key = plugin + '#' + method;
+    if (listeners[key]) {
+      const responder = new Responder(responderId);
+      listeners[key](JSON.parse(params), responder);
+    }
+  });
+}
+
 // $FlowFixMe
 export function addPlugin(plugin) {
   if (!plugin || typeof plugin !== 'object') {
@@ -83,18 +119,10 @@ export function addPlugin(plugin) {
       ? !!plugin.runInBackground()
       : false;
   const id = plugin.getId();
-  const connection = new Connection(id);
+  plugin._connection = new Connection(id);
+  plugins[id] = plugin;
 
-  Flipper.registerPlugin(
+  Flipper.registerPlugin(id, runInBackground);
-    id,
-    runInBackground,
-    function onConnect() {
-      connection.connected = true;
-      plugin.onConnect(connection);
-    },
-    function onDisconnect() {
-      connection.connected = false;
-      plugin.onDisconnect();
-    },
-  );
 }
+
+startEventListeners();

src/chrome/mainsidebar/MainSidebar2.tsx

@@ -71,7 +71,7 @@ const SidebarSectionButton = styled('button')<{
   level: SectionLevel;
   color: string;
   collapsed: boolean;
-}>(({level, color, collapsed}) => ({
+}>(({level, color}) => ({
   fontWeight: level === 3 ? 'normal' : 'bold',
   borderRadius: 0,
   border: 'none',