Introduce onDestroy hook

Summary: This diff introduces the `onDestroy` hook that can be used by plugins to listen to the event where a plugin is cleaned up (either because it is disabled, or because the client is being cleaned up)

Reviewed By: jknoxville

Differential Revision: D22208121

fbshipit-source-id: 9c4951ae671be611f21da171c548d4054c481166

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

@@ -11,7 +11,11 @@ import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWit
 import {Store, Client} from '../../';
 import {selectPlugin, starPlugin} from '../../reducers/connections';
 import {registerPlugins} from '../../reducers/plugins';
-import {SandyPluginDefinition, SandyPluginInstance} from 'flipper-plugin';
+import {
+  SandyPluginDefinition,
+  SandyPluginInstance,
+  FlipperClient,
+} from 'flipper-plugin';
 
 interface PersistedState {
   count: 1;
@@ -33,8 +37,17 @@ const pluginDetails = {
 let TestPlugin: SandyPluginDefinition;
 
 beforeEach(() => {
+  function plugin(client: FlipperClient) {
+    const destroyStub = jest.fn();
+
+    client.onDestroy(destroyStub);
+
+    return {
+      destroyStub,
+    };
+  }
   TestPlugin = new SandyPluginDefinition(pluginDetails, {
-    plugin: jest.fn().mockImplementation(() => ({})),
+    plugin: jest.fn().mockImplementation(plugin) as typeof plugin,
     Component: jest.fn().mockImplementation(() => null),
   });
 });
@@ -76,14 +89,25 @@ 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(
+  const pluginInstance = client.sandyPluginStates.get(TestPlugin.id)!;
-    SandyPluginInstance,
+  expect(pluginInstance).toBeInstanceOf(SandyPluginInstance);
-  );
+  expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(0);
 
   // unstar
   starTestPlugin(store, client);
-  expect(client.sandyPluginStates.get(TestPlugin.id)).toBeUndefined();
+  expect(client.sandyPluginStates.has(TestPlugin.id)).toBeFalsy();
-  // TODO: make sure onDestroy is  called T68683507
+  expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(1);
+});
+
+test('it should cleanup if client is removed', async () => {
+  const {client} = await createMockFlipperWithPlugin(TestPlugin);
+  const pluginInstance = client.sandyPluginStates.get(TestPlugin.id)!;
+  expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(0);
+
+  // close client
+  client.close();
+  expect(client.sandyPluginStates.has(TestPlugin.id)).toBeFalsy();
+  expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(1);
 });
 
 test('it should not initialize a sandy plugin if not enabled', async () => {
@@ -96,7 +120,11 @@ test('it should not initialize a sandy plugin if not enabled', async () => {
       id: 'Plugin2',
     },
     {
-      plugin: jest.fn().mockImplementation(() => ({})),
+      plugin: jest.fn().mockImplementation((client) => {
+        const destroyStub = jest.fn();
+        client.onDestroy(destroyStub);
+        return {destroyStub};
+      }),
       Component() {
         return null;
       },
@@ -121,10 +149,13 @@ test('it should not initialize a sandy plugin if not enabled', async () => {
   expect(client.sandyPluginStates.get(Plugin2.id)).toBeInstanceOf(
     SandyPluginInstance,
   );
+  const destroyStub = client.sandyPluginStates.get(Plugin2.id)!.instanceApi
+    .destroyStub;
   expect(client.sandyPluginStates.get(TestPlugin.id)).toBe(pluginState1); // not reinitialized
 
   expect(TestPlugin.module.plugin).toBeCalledTimes(1);
   expect(Plugin2.module.plugin).toBeCalledTimes(1);
+  expect(destroyStub).toHaveBeenCalledTimes(0);
 
   // disable plugin again
   store.dispatch(
@@ -135,7 +166,7 @@ test('it should not initialize a sandy plugin if not enabled', async () => {
   );
 
   expect(client.sandyPluginStates.get(Plugin2.id)).toBeUndefined();
-  // TODO: test destroy hook is called T68683507
+  expect(destroyStub).toHaveBeenCalledTimes(1);
 });
 
 // TODO: T68683449 state is persisted if a plugin connects and reconnects

desktop/flipper-plugin/src/index.ts

@@ -8,3 +8,4 @@
  */
 
 export * from './plugin/Plugin';
+export * from './plugin/SandyPluginDefinition';

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

@@ -7,7 +7,8 @@
  * @format
  */
 
-import {PluginDetails} from 'flipper-plugin-lib';
+import {SandyPluginDefinition} from './SandyPluginDefinition';
+import {EventEmitter} from 'events';
 
 type EventsContract = Record<string, any>;
 type MethodsContract = Record<string, (params: any) => Promise<any>>;
@@ -16,13 +17,18 @@ type MethodsContract = Record<string, (params: any) => Promise<any>>;
  * API available to a plugin factory
  */
 export interface FlipperClient<
-  Events extends EventsContract,
+  Events extends EventsContract = {},
-  Methods extends MethodsContract
+  Methods extends MethodsContract = {}
-> {}
+> {
+  /**
+   * the onDestroy event is fired whenever a client is unloaded from Flipper, or a plugin is disabled.
+   */
+  onDestroy(cb: () => void): void;
+}
 
 /**
  * Internal API exposed by Flipper, and wrapped by FlipperPluginInstance to be passed to the
- * Plugin Factory
+ * Plugin Factory. For internal purposes only
  */
 interface RealFlipperClient {}
 
@@ -33,15 +39,6 @@ export type FlipperPluginFactory<
 
 export type FlipperPluginComponent = React.FC<{}>;
 
-export type FlipperPluginModule = {
-  /** the factory function that initializes a plugin instance */
-  plugin: FlipperPluginFactory<any, any>;
-  /** the component type that can render this plugin */
-  Component: FlipperPluginComponent;
-  // TODO: support device plugins T68738317
-  // devicePlugin: FlipperPluginFactory
-};
-
 export class SandyPluginInstance {
   /** base client provided by Flipper */
   realClient: RealFlipperClient;
@@ -50,7 +47,9 @@ export class SandyPluginInstance {
   /** the original plugin definition */
   definition: SandyPluginDefinition;
   /** the plugin instance api as used inside components and such  */
-  instanceApi: object;
+  instanceApi: any;
+
+  events = new EventEmitter();
 
   constructor(
     realClient: RealFlipperClient,
@@ -58,7 +57,11 @@ export class SandyPluginInstance {
   ) {
     this.realClient = realClient;
     this.definition = definition;
-    this.client = {};
+    this.client = {
+      onDestroy: (cb) => {
+        this.events.on('destroy', cb);
+      },
+    };
     this.instanceApi = definition.module.plugin(this.client);
   }
 
@@ -71,84 +74,10 @@ export class SandyPluginInstance {
   }
 
   destroy() {
-    // TODO: T68683507
+    this.events.emit('destroy');
   }
 
   toJSON() {
     // TODO: T68683449
   }
 }
-
-/**
- * A sandy plugin definitions represents a loaded plugin definition, storing two things:
- * the loaded JS module, and the meta data (typically coming from package.json).
- *
- * Also delegates some of the standard plugin functionality to have a similar public static api as FlipperPlugin
- */
-export class SandyPluginDefinition {
-  id: string;
-  module: FlipperPluginModule;
-  details: PluginDetails;
-
-  // TODO: Implement T68683449
-  exportPersistedState:
-    | ((
-        callClient: (method: string, params?: any) => Promise<any>,
-        persistedState: any, // TODO: type StaticPersistedState | undefined,
-        store: any, // TODO: ReduxState | undefined,
-        idler?: any, // TODO: Idler,
-        statusUpdate?: (msg: string) => void,
-        supportsMethod?: (method: string) => Promise<boolean>,
-      ) => Promise<any /* TODO: StaticPersistedState | undefined */>)
-    | undefined = undefined;
-
-  constructor(details: PluginDetails, module: FlipperPluginModule) {
-    this.id = details.id;
-    this.details = details;
-    if (!module.plugin || typeof module.plugin !== 'function') {
-      throw new Error(
-        `Flipper plugin '${this.id}' should export named function called 'plugin'`,
-      );
-    }
-    if (!module.Component || typeof module.Component !== 'function') {
-      throw new Error(
-        `Flipper plugin '${this.id}' should export named function called 'Component'`,
-      );
-    }
-    this.module = module;
-    this.module.Component.displayName = `FlipperPlugin(${this.id})`;
-  }
-
-  get packageName() {
-    return this.details.name;
-  }
-
-  get title() {
-    return this.details.title;
-  }
-
-  get icon() {
-    return this.details.icon;
-  }
-
-  get category() {
-    return this.details.category;
-  }
-
-  get gatekeeper() {
-    return this.details.gatekeeper;
-  }
-
-  get version() {
-    return this.details.version;
-  }
-
-  get isDefault() {
-    return this.details.isDefault;
-  }
-
-  get keyboardActions() {
-    // TODO: T68882551 support keyboard actions
-    return [];
-  }
-}

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

@@ -0,0 +1,97 @@
+/**
+ * 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 {PluginDetails} from 'flipper-plugin-lib';
+import {FlipperPluginFactory, FlipperPluginComponent} from './Plugin';
+
+/**
+ * FlipperPluginModule describe the exports that are provided by a typical Flipper Desktop plugin
+ */
+export type FlipperPluginModule = {
+  /** the factory function that initializes a plugin instance */
+  plugin: FlipperPluginFactory<any, any>;
+  /** the component type that can render this plugin */
+  Component: FlipperPluginComponent;
+  // TODO: support device plugins T68738317
+  // devicePlugin: FlipperPluginFactory
+};
+
+/**
+ * A sandy plugin definitions represents a loaded plugin definition, storing two things:
+ * the loaded JS module, and the meta data (typically coming from package.json).
+ *
+ * Also delegates some of the standard plugin functionality to have a similar public static api as FlipperPlugin
+ */
+export class SandyPluginDefinition {
+  id: string;
+  module: FlipperPluginModule;
+  details: PluginDetails;
+
+  // TODO: Implement T68683449
+  exportPersistedState:
+    | ((
+        callClient: (method: string, params?: any) => Promise<any>,
+        persistedState: any, // TODO: type StaticPersistedState | undefined,
+        store: any, // TODO: ReduxState | undefined,
+        idler?: any, // TODO: Idler,
+        statusUpdate?: (msg: string) => void,
+        supportsMethod?: (method: string) => Promise<boolean>,
+      ) => Promise<any /* TODO: StaticPersistedState | undefined */>)
+    | undefined = undefined;
+
+  constructor(details: PluginDetails, module: FlipperPluginModule) {
+    this.id = details.id;
+    this.details = details;
+    if (!module.plugin || typeof module.plugin !== 'function') {
+      throw new Error(
+        `Flipper plugin '${this.id}' should export named function called 'plugin'`,
+      );
+    }
+    if (!module.Component || typeof module.Component !== 'function') {
+      throw new Error(
+        `Flipper plugin '${this.id}' should export named function called 'Component'`,
+      );
+    }
+    this.module = module;
+    this.module.Component.displayName = `FlipperPlugin(${this.id})`;
+  }
+
+  get packageName() {
+    return this.details.name;
+  }
+
+  get title() {
+    return this.details.title;
+  }
+
+  get icon() {
+    return this.details.icon;
+  }
+
+  get category() {
+    return this.details.category;
+  }
+
+  get gatekeeper() {
+    return this.details.gatekeeper;
+  }
+
+  get version() {
+    return this.details.version;
+  }
+
+  get isDefault() {
+    return this.details.isDefault;
+  }
+
+  get keyboardActions() {
+    // TODO: T68882551 support keyboard actions
+    return [];
+  }
+}