Make sure Sandy plugis can be initialized

Summary:
This diff makes sure sandy plugins are initialized.

Sandy plugins are stored directly in the client for two reasons
1. we want to decouple any plugin state updates from our central redux store, as redux is particularly bad in handling high frequency updates.
2. The lifecycle of a plugin is now bound to the lifecycle of a client. This means that we don't need 'persistedStore' to make sure state is preserved; that is now the default. Switching plugins will no longer reinitialize them (but only run specific hooks, see later diffs).
3. PersistedState will be introduced for sandy plugins as well later, but primarily for import / export / debug reasons.

A significant difference with the current persistent state, is that if a client crashes and reconnects, plugins will loose their state. We can prevent this (again, since state persisting will be reintroduced), but I'm not sure we need that for the specific reconnect scenario. Because
1. we should fix reconnecting clients anyway, and from stats it looks to happen less already
2. reconnects are usually caused by plugins that aggregate a lot of data and get slower over time. Restoring old state also restores those unstabilites.

For the overview bringing back the archi picture of earlier diff:
{F241508042}

Also fixed a bug where enabling background plugins didn't enable them on all devices with that app.

Reviewed By: jknoxville

Differential Revision: D22186276

fbshipit-source-id: 3fd42b577f86920e5280aa8cce1a0bc4d5564ed9

desktop/app/src/Client.tsx

@@ -32,7 +32,7 @@ import {sideEffect} from './utils/sideEffect';
 import {emitBytesReceived} from './dispatcher/tracking';
 import {debounce} from 'lodash';
 import {batch} from 'react-redux';
-import {SandyPluginDefinition} from 'flipper-plugin';
+import {SandyPluginDefinition, SandyPluginInstance} from 'flipper-plugin';
 
 type Plugins = Array<string>;
 
@@ -135,6 +135,7 @@ export default class Client extends EventEmitter {
       messages: Params[];
     }
   > = {};
+  sandyPluginStates = new Map<string /*pluginID*/, SandyPluginInstance>();
 
   requestCallbacks: Map<
     number,
@@ -253,18 +254,26 @@ export default class Client extends EventEmitter {
     return this.backgroundPlugins.includes(pluginId);
   }
 
+  isEnabledPlugin(pluginId: string) {
+    return this.store
+      .getState()
+      .connections.userStarredPlugins[this.query.app]?.includes(pluginId);
+  }
+
   shouldConnectAsBackgroundPlugin(pluginId: string) {
     return (
       defaultEnabledBackgroundPlugins.includes(pluginId) ||
-      this.store
+      this.isEnabledPlugin(pluginId)
-        .getState()
-        .connections.userStarredPlugins[this.query.app]?.includes(pluginId)
     );
   }
 
   async init() {
     this.setMatchingDevice();
     await this.loadPlugins();
+    // this starts all sandy enabled plugin
+    this.plugins.forEach((pluginId) =>
+      this.startPluginIfNeeded(this.getPlugin(pluginId)),
+    );
     this.backgroundPlugins = await this.getBackgroundPlugins();
     this.backgroundPlugins.forEach((plugin) => {
       if (this.shouldConnectAsBackgroundPlugin(plugin)) {
@@ -298,6 +307,47 @@ export default class Client extends EventEmitter {
     return plugins;
   }
 
+  startPluginIfNeeded(
+    plugin: PluginDefinition | undefined,
+    isEnabled = plugin ? this.isEnabledPlugin(plugin.id) : false,
+  ) {
+    // start a plugin on start if it is a SandyPlugin, which is starred, and doesn't have persisted state yet
+    if (
+      plugin instanceof SandyPluginDefinition &&
+      isEnabled &&
+      !this.sandyPluginStates.has(plugin.id)
+    ) {
+      // TODO: needs to be wrapped in error tracking T68955280
+      // TODO: pick up any existing persisted state T68683449
+      this.sandyPluginStates.set(
+        plugin.id,
+        new SandyPluginInstance(this, plugin),
+      );
+    }
+  }
+
+  stopPluginIfNeeded(pluginId: string) {
+    const instance = this.sandyPluginStates.get(pluginId);
+    if (instance) {
+      instance.destroy();
+      // TODO: make sure persisted state is writtenT68683449
+      this.sandyPluginStates.delete(pluginId);
+    }
+  }
+
+  close() {
+    this.emit('close');
+    this.plugins.forEach((pluginId) => this.stopPluginIfNeeded(pluginId));
+  }
+
+  // gets a plugin by pluginId
+  getPlugin(pluginId: string): PluginDefinition | undefined {
+    const plugins = this.store.getState().plugins;
+    return (
+      plugins.clientPlugins.get(pluginId) || plugins.devicePlugins.get(pluginId)
+    );
+  }
+
   // get the supported background plugins
   async getBackgroundPlugins(): Promise<Plugins> {
     if (this.sdkVersion < 4) {
@@ -313,6 +363,9 @@ export default class Client extends EventEmitter {
   async refreshPlugins() {
     const oldBackgroundPlugins = this.backgroundPlugins;
     await this.loadPlugins();
+    this.plugins.forEach((pluginId) =>
+      this.startPluginIfNeeded(this.getPlugin(pluginId)),
+    );
     const newBackgroundPlugins = await this.getBackgroundPlugins();
     this.backgroundPlugins = newBackgroundPlugins;
     // diff the background plugin list, disconnect old, connect new ones
@@ -616,9 +669,11 @@ export default class Client extends EventEmitter {
   initPlugin(pluginId: string) {
     this.activePlugins.add(pluginId);
     this.rawSend('init', {plugin: pluginId});
+    // TODO: call sandyOnConnect
   }
 
   deinitPlugin(pluginId: string) {
+    // TODO: call sandyOnDisconnect
     this.activePlugins.delete(pluginId);
     this.rawSend('deinit', {plugin: pluginId});
   }

desktop/app/src/PluginContainer.tsx

@@ -274,7 +274,7 @@ class PluginContainer extends PureComponent<Props, State> {
               toggled={false}
               onClick={() => {
                 this.props.starPlugin({
-                  selectedPlugin: activePlugin.id,
+                  plugin: activePlugin,
                   selectedApp: (this.props.target as Client).query.app,
                 });
               }}

desktop/app/src/chrome/mainsidebar/MainSidebar2.tsx

@@ -486,10 +486,10 @@ const PluginList = memo(function PluginList({
   }, [client]);
 
   const onFavorite = useCallback(
-    (plugin: string) => {
+    (plugin: PluginDefinition) => {
       starPlugin({
         selectedApp: client.query.app,
-        selectedPlugin: plugin,
+        plugin,
       });
     },
     [client],
@@ -589,7 +589,7 @@ const PluginsByCategory = memo(function PluginsByCategory({
   starred: boolean;
   selectedPlugin?: null | string;
   selectedApp?: null | string;
-  onFavorite: (pluginId: string) => void;
+  onFavorite: (plugin: PluginDefinition) => void;
   selectPlugin: SelectPlugin;
 }) {
   return (
@@ -617,7 +617,7 @@ const PluginsByCategory = memo(function PluginsByCategory({
               }
               plugin={plugin}
               app={client.query.app}
-              onFavorite={() => onFavorite(plugin.id)}
+              onFavorite={() => onFavorite(plugin)}
               starred={device.isArchived ? undefined : starred}
             />
           ))}

desktop/app/src/reducers/__tests__/sandyplugins.node.tsx

@@ -0,0 +1,141 @@
+/**
+ * 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.
+ *
+ * @format
+ */
+
+import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin';
+import {Store, Client} from '../../';
+import {selectPlugin, starPlugin} from '../../reducers/connections';
+import {registerPlugins} from '../../reducers/plugins';
+import {SandyPluginDefinition, SandyPluginInstance} from 'flipper-plugin';
+
+interface PersistedState {
+  count: 1;
+}
+
+const pluginDetails = {
+  id: 'TestPlugin',
+  dir: '',
+  name: 'TestPlugin',
+  specVersion: 0,
+  entry: '',
+  isDefault: false,
+  main: '',
+  source: '',
+  title: 'Testing Plugin',
+  version: '',
+} as const;
+
+let TestPlugin: SandyPluginDefinition;
+
+beforeEach(() => {
+  TestPlugin = new SandyPluginDefinition(pluginDetails, {
+    plugin: jest.fn().mockImplementation(() => ({})),
+    Component: jest.fn().mockImplementation(() => null),
+  });
+});
+
+function starTestPlugin(store: Store, client: Client) {
+  store.dispatch(
+    starPlugin({
+      plugin: TestPlugin,
+      selectedApp: client.query.app,
+    }),
+  );
+}
+
+function selectTestPlugin(store: Store, client: Client) {
+  store.dispatch(
+    selectPlugin({
+      selectedPlugin: TestPlugin.id,
+      selectedApp: client.query.app,
+      deepLinkPayload: null,
+      selectedDevice: store.getState().connections.selectedDevice!,
+    }),
+  );
+}
+
+test('it should initialize starred sandy plugins', async () => {
+  const {client, store} = await createMockFlipperWithPlugin(TestPlugin);
+
+  // already started, so initialized immediately
+  expect(TestPlugin.module.plugin).toBeCalledTimes(1);
+  expect(client.sandyPluginStates.get(TestPlugin.id)).toBeInstanceOf(
+    SandyPluginInstance,
+  );
+
+  selectTestPlugin(store, client);
+  // TODO: make sure lifecycle 'activated' or something is triggered T68683507
+});
+
+test('it should cleanup a plugin if disabled', async () => {
+  const {client, store} = await createMockFlipperWithPlugin(TestPlugin);
+
+  expect(TestPlugin.module.plugin).toBeCalledTimes(1);
+  expect(client.sandyPluginStates.get(TestPlugin.id)).toBeInstanceOf(
+    SandyPluginInstance,
+  );
+
+  // unstar
+  starTestPlugin(store, client);
+  expect(client.sandyPluginStates.get(TestPlugin.id)).toBeUndefined();
+  // TODO: make sure onDestroy is  called T68683507
+});
+
+test('it should not initialize a sandy plugin if not enabled', async () => {
+  const {client, store} = await createMockFlipperWithPlugin(TestPlugin);
+
+  const Plugin2 = new SandyPluginDefinition(
+    {
+      ...pluginDetails,
+      name: 'Plugin2',
+      id: 'Plugin2',
+    },
+    {
+      plugin: jest.fn().mockImplementation(() => ({})),
+      Component() {
+        return null;
+      },
+    },
+  );
+
+  const pluginState1 = client.sandyPluginStates.get(TestPlugin.id);
+  expect(pluginState1).toBeInstanceOf(SandyPluginInstance);
+  store.dispatch(registerPlugins([Plugin2]));
+  await client.refreshPlugins();
+  // not yet enabled, so not yet started
+  expect(client.sandyPluginStates.get(Plugin2.id)).toBeUndefined();
+  expect(Plugin2.module.plugin).toBeCalledTimes(0);
+
+  store.dispatch(
+    starPlugin({
+      plugin: Plugin2,
+      selectedApp: client.query.app,
+    }),
+  );
+
+  expect(client.sandyPluginStates.get(Plugin2.id)).toBeInstanceOf(
+    SandyPluginInstance,
+  );
+  expect(client.sandyPluginStates.get(TestPlugin.id)).toBe(pluginState1); // not reinitialized
+
+  expect(TestPlugin.module.plugin).toBeCalledTimes(1);
+  expect(Plugin2.module.plugin).toBeCalledTimes(1);
+
+  // disable plugin again
+  store.dispatch(
+    starPlugin({
+      plugin: Plugin2,
+      selectedApp: client.query.app,
+    }),
+  );
+
+  expect(client.sandyPluginStates.get(Plugin2.id)).toBeUndefined();
+  // TODO: test destroy hook is called T68683507
+});
+
+// TODO: T68683449 state is persisted if a plugin connects and reconnects

desktop/app/src/reducers/connections.tsx

@@ -28,7 +28,7 @@ import {
   defaultEnabledBackgroundPlugins,
 } from '../utils/pluginUtils';
 import {deconstructClientId} from '../utils/clientUtils';
-import {FlipperDevicePlugin} from '../plugin';
+import {FlipperDevicePlugin, PluginDefinition} from '../plugin';
 import {RegisterPluginAction} from './plugins';
 import {SandyPluginDefinition} from 'flipper-plugin';
 
@@ -138,8 +138,8 @@ export type Action =
   | {
       type: 'STAR_PLUGIN';
       payload: {
-        selectedPlugin: string;
         selectedApp: string;
+        plugin: PluginDefinition;
       };
     }
   | {
@@ -279,33 +279,46 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
     }
 
     case 'STAR_PLUGIN': {
-      const {selectedPlugin, selectedApp} = action.payload;
+      const {plugin, selectedApp} = action.payload;
-      const client = state.clients.find(
+      const selectedPlugin = plugin.id;
+      const clients = state.clients.filter(
         (client) => client.query.app === selectedApp,
       );
       return produce(state, (draft) => {
         if (!draft.userStarredPlugins[selectedApp]) {
-          draft.userStarredPlugins[selectedApp] = [selectedPlugin];
+          draft.userStarredPlugins[selectedApp] = [];
-        } else {
+        }
-          const plugins = draft.userStarredPlugins[selectedApp];
+        const plugins = draft.userStarredPlugins[selectedApp];
-          const idx = plugins.indexOf(selectedPlugin);
+        const idx = plugins.indexOf(selectedPlugin);
-          if (idx === -1) {
+        if (idx === -1) {
-            plugins.push(selectedPlugin);
+          plugins.push(selectedPlugin);
+          // enabling a plugin on one device enables it on all...
+          clients.forEach((client) => {
+            // sandy plugin? initialize it
+            client.startPluginIfNeeded(plugin, true);
+            // background plugin? connect it needed
             if (
               !defaultEnabledBackgroundPlugins.includes(selectedPlugin) &&
               client?.isBackgroundPlugin(selectedPlugin)
             ) {
               client.initPlugin(selectedPlugin);
             }
-          } else {
+          });
-            plugins.splice(idx, 1);
+        } else {
+          plugins.splice(idx, 1);
+          // enabling a plugin on one device disables it on all...
+          clients.forEach((client) => {
+            // disconnect background plugins
             if (
               !defaultEnabledBackgroundPlugins.includes(selectedPlugin) &&
               client?.isBackgroundPlugin(selectedPlugin)
             ) {
               client.deinitPlugin(selectedPlugin);
             }
-          }
+            // stop sandy plugins
+            // TODO: forget any persisted state as well T68683449
+            client.stopPluginIfNeeded(plugin.id);
+          });
         }
       });
     }
@@ -500,7 +513,7 @@ export const selectPlugin = (payload: {
 });
 
 export const starPlugin = (payload: {
-  selectedPlugin: string;
+  plugin: PluginDefinition;
   selectedApp: string;
 }): Action => ({
   type: 'STAR_PLUGIN',

desktop/app/src/reducers/supportForm.tsx

@@ -134,10 +134,13 @@ export class Group {
           selectedClient.plugins.includes(requiredPlugin) &&
           !requiredPluginEnabled
         ) {
+          const plugin =
+            store.getState().plugins.clientPlugins.get(requiredPlugin) ||
+            store.getState().plugins.devicePlugins.get(requiredPlugin)!;
           store.dispatch(
             setStarPlugin({
               selectedApp: app,
-              selectedPlugin: requiredPlugin,
+              plugin,
             }),
           );
         } else if (

desktop/app/src/server.tsx

@@ -507,7 +507,7 @@ class Server extends EventEmitter {
   removeConnection(id: string) {
     const info = this.connections.get(id);
     if (info) {
-      info.client.emit('close');
+      info.client.close();
       this.connections.delete(id);
       this.emit('clients-change');
       this.emit('removed-client', id);

desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx

@@ -29,33 +29,28 @@ import Client, {ClientQuery} from '../Client';
 
 import {buildClientId} from '../utils/clientUtils';
 import {Logger} from '../fb-interfaces/Logger';
-import {FlipperPlugin} from '../plugin';
+import {FlipperPlugin, PluginDefinition} from '../plugin';
 import {registerPlugins} from '../reducers/plugins';
 import PluginContainer from '../PluginContainer';
-
+import {getPluginKey} from '../utils/pluginUtils';
-export function createStubLogger(): Logger {
+import {getInstance} from '../fb-stubs/Logger';
-  return {
-    ...console,
-    track: console.info,
-    trackTimeSince: console.info,
-  };
-}
 
 type MockFlipperResult = {
   client: Client;
   device: BaseDevice;
   store: Store;
-  sendMessage(method: string, params: any): void;
+  pluginKey: string;
+  sendMessage(method: string, params: any, client?: Client): void;
   createDevice(serial: string): BaseDevice;
-  createClient(device: BaseDevice, name: string): Client;
+  createClient(device: BaseDevice, name: string): Promise<Client>;
   logger: Logger;
 };
 
 export async function createMockFlipperWithPlugin(
-  pluginClazz: typeof FlipperPlugin,
+  pluginClazz: PluginDefinition,
 ): Promise<MockFlipperResult> {
   const store = createStore(reducers);
-  const logger = createStubLogger();
+  const logger = getInstance();
   store.dispatch(registerPlugins([pluginClazz]));
 
   function createDevice(serial: string): BaseDevice {
@@ -72,7 +67,10 @@ export async function createMockFlipperWithPlugin(
     return device;
   }
 
-  function createClient(device: BaseDevice, name: string): Client {
+  async function createClient(
+    device: BaseDevice,
+    name: string,
+  ): Promise<Client> {
     const query: ClientQuery = {
       app: name,
       os: 'Android',
@@ -102,6 +100,38 @@ export async function createMockFlipperWithPlugin(
         return device;
       },
     } as any;
+    client.rawCall = async (method, _fromPlugin, _params): Promise<any> => {
+      // TODO: could use an interceptor here
+      switch (method) {
+        case 'getPlugins':
+          // assuming this plugin supports all plugins for now
+          return {
+            plugins: [
+              ...store.getState().plugins.clientPlugins.keys(),
+              ...store.getState().plugins.devicePlugins.keys(),
+            ],
+          };
+        default:
+          throw new Error(`Test client doesn't supoprt rawCall to ${method}`);
+      }
+    };
+
+    // enable the plugin
+    if (
+      !store
+        .getState()
+        .connections.userStarredPlugins[client.query.app]?.includes(
+          pluginClazz.id,
+        )
+    ) {
+      store.dispatch(
+        starPlugin({
+          plugin: pluginClazz,
+          selectedApp: client.query.app,
+        }),
+      );
+    }
+    await client.init();
 
     // As convenience, by default we select the new client, star the plugin, and select it
     store.dispatch({
@@ -112,18 +142,11 @@ export async function createMockFlipperWithPlugin(
   }
 
   const device = createDevice('serial');
-  const client = createClient(device, 'TestApp');
+  const client = await createClient(device, 'TestApp');
 
   store.dispatch(selectDevice(device));
   store.dispatch(selectClient(client.id));
 
-  store.dispatch(
-    starPlugin({
-      selectedPlugin: pluginClazz.id,
-      selectedApp: client.query.app,
-    }),
-  );
-
   store.dispatch(
     selectPlugin({
       selectedPlugin: pluginClazz.id,
@@ -137,8 +160,8 @@ export async function createMockFlipperWithPlugin(
     client,
     device: device as any,
     store,
-    sendMessage(method, params) {
+    sendMessage(method, params, actualClient = client) {
-      client.onMessage(
+      actualClient.onMessage(
         JSON.stringify({
           method: 'execute',
           params: {
@@ -152,6 +175,7 @@ export async function createMockFlipperWithPlugin(
     createDevice,
     createClient,
     logger,
+    pluginKey: getPluginKey(client.id, device, pluginClazz.id),
   };
 }
 

desktop/app/src/utils/__tests__/messageQueue.node.tsx

@@ -57,7 +57,7 @@ class TestPlugin extends FlipperPlugin<any, any, any> {
 function starTestPlugin(store: Store, client: Client) {
   store.dispatch(
     starPlugin({
-      selectedPlugin: TestPlugin.id,
+      plugin: TestPlugin,
       selectedApp: client.query.app,
     }),
   );
@@ -214,7 +214,7 @@ test('queue - events are queued for plugins that are favorite when app is not se
   selectDeviceLogs(store);
   expect(store.getState().connections.selectedPlugin).not.toBe('TestPlugin');
 
-  const client2 = createClient(device, 'TestApp2');
+  const client2 = await createClient(device, 'TestApp2');
   store.dispatch(selectClient(client2.id));
 
   // Now we send a message to the second client, it should arrive,
@@ -248,27 +248,39 @@ test('queue - events are queued for plugins that are favorite when app is select
   expect(store.getState().connections.selectedPlugin).not.toBe('TestPlugin');
 
   const device2 = createDevice('serial2');
-  const client2 = createClient(device2, client.query.app); // same app id
+  const client2 = await createClient(device2, client.query.app); // same app id
   store.dispatch(selectDevice(device2));
   store.dispatch(selectClient(client2.id));
 
-  // Now we send a message to the second client, it should arrive,
+  // Now we send a message to the first and second client, it should arrive,
   // as the plugin was enabled already on the first client as well
   sendMessage('inc', {delta: 2});
+  sendMessage('inc', {delta: 3}, client2);
+  client.flushMessageBuffer();
+  client2.flushMessageBuffer();
   expect(store.getState().pluginStates).toMatchInlineSnapshot(`Object {}`);
   expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot(`
-          Object {
+    Object {
-            "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Array [
+      "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Array [
-              Object {
+        Object {
-                "api": "TestPlugin",
+          "api": "TestPlugin",
-                "method": "inc",
+          "method": "inc",
-                "params": Object {
+          "params": Object {
-                  "delta": 2,
+            "delta": 2,
-                },
+          },
-              },
+        },
-            ],
+      ],
-          }
+      "TestApp#Android#MockAndroidDevice#serial2#TestPlugin": Array [
-        `);
+        Object {
+          "api": "TestPlugin",
+          "method": "inc",
+          "params": Object {
+            "delta": 3,
+          },
+        },
+      ],
+    }
+  `);
 });
 
 test('queue - events processing will be paused', async () => {

desktop/flipper-plugin/src/plugin/Plugin.tsx

@@ -42,25 +42,40 @@ export type FlipperPluginModule = {
   // devicePlugin: FlipperPluginFactory
 };
 
-export class FlipperPluginInstance {
+export class SandyPluginInstance {
   /** base client provided by Flipper */
   realClient: RealFlipperClient;
   /** client that is bound to this instance */
   client: FlipperClient<any, any>;
   /** the original plugin definition */
-  definition: FlipperPluginModule;
+  definition: SandyPluginDefinition;
   /** the plugin instance api as used inside components and such  */
   instanceApi: object;
 
-  constructor(realClient: RealFlipperClient, definition: FlipperPluginModule) {
+  constructor(
+    realClient: RealFlipperClient,
+    definition: SandyPluginDefinition,
+  ) {
     this.realClient = realClient;
     this.definition = definition;
     this.client = {};
-    this.instanceApi = definition.plugin(this.client);
+    this.instanceApi = definition.module.plugin(this.client);
+  }
+
+  activate() {
+    // TODO: T68683507
   }
 
   deactivate() {
-    // TODO:
+    // TODO: T68683507
+  }
+
+  destroy() {
+    // TODO: T68683507
+  }
+
+  toJSON() {
+    // TODO: T68683449
   }
 }