From b9c9e89b532ac1dbf20efceae2bd3c338c59c137 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 4 Aug 2020 07:05:57 -0700 Subject: [PATCH] Support `activate` and `deactivate` in normal plugins Summary: Device plugins have an activate and deactivate hook, that reflects the plugin being selected in the UI. Added these same hooks to client plugins as well. In practice they are called at the same times as `onConnect` and `onDisconnect`, except for background plugins, which connect only once, so it is pretty useful to be still able to make the distinction. Since there is now quite some common functionality between plugins and device plugins, will clean things a bit up in a next diff [Interesting] as it explains the difference between the different lifecycle methods of plugins, and the impact of being a background plugin LIfecycle summary: 1. app connects 2. for background plugins: connect them (`onConnect`) 3. user selects a plugin, triggers `onActivate`. will also trigger `onConnect` the plugin if it _isnt_ a bg plugin 4. user selects a different plugin, triggers `onDeactivate`. will also trigger `onDisconnect` if it isn't a bg plugin. 5. app is unloaded. Triggers `onDisconnect` for bg plugins. Triggers `onDestroy` for all plugins, Reviewed By: jknoxville Differential Revision: D22724791 fbshipit-source-id: 9fe2e666eb37fa2e0bd00fa61d78d2d4b1080137 --- desktop/app/src/PluginContainer.tsx | 1 - .../src/__tests__/PluginContainer.node.tsx | 162 +++++++++++++++++- desktop/app/src/index.tsx | 5 +- .../createMockFlipperWithPlugin.tsx | 3 +- .../src/__tests__/TestPlugin.tsx | 6 + .../src/__tests__/test-utils.node.tsx | 52 +++++- .../src/plugin/DevicePlugin.tsx | 5 +- desktop/flipper-plugin/src/plugin/Plugin.tsx | 36 +++- .../src/test-utils/test-utils.tsx | 37 +++- desktop/plugins/logs/index.tsx | 2 +- 10 files changed, 288 insertions(+), 21 deletions(-) diff --git a/desktop/app/src/PluginContainer.tsx b/desktop/app/src/PluginContainer.tsx index 23b1c8334..d075288c4 100644 --- a/desktop/app/src/PluginContainer.tsx +++ b/desktop/app/src/PluginContainer.tsx @@ -248,7 +248,6 @@ class PluginContainer extends PureComponent { pluginIsEnabled, } = this.props; if (!activePlugin || !target || !pluginKey) { - console.warn(`No selected plugin. Rendering empty!`); return null; } diff --git a/desktop/app/src/__tests__/PluginContainer.node.tsx b/desktop/app/src/__tests__/PluginContainer.node.tsx index d09b0646a..e139838e2 100644 --- a/desktop/app/src/__tests__/PluginContainer.node.tsx +++ b/desktop/app/src/__tests__/PluginContainer.node.tsx @@ -101,6 +101,8 @@ test('PluginContainer can render Sandy plugins', async () => { expect(Object.keys(sandyApi)).toEqual([ 'connectedStub', 'disconnectedStub', + 'activatedStub', + 'deactivatedStub', ]); expect(() => { // eslint-disable-next-line @@ -114,9 +116,13 @@ test('PluginContainer can render Sandy plugins', async () => { const plugin = (client: FlipperClient) => { const connectedStub = jest.fn(); const disconnectedStub = jest.fn(); + const activatedStub = jest.fn(); + const deactivatedStub = jest.fn(); client.onConnect(connectedStub); client.onDisconnect(disconnectedStub); - return {connectedStub, disconnectedStub}; + client.onActivate(activatedStub); + client.onDeactivate(deactivatedStub); + return {connectedStub, disconnectedStub, activatedStub, deactivatedStub}; }; const definition = new SandyPluginDefinition( @@ -167,6 +173,8 @@ test('PluginContainer can render Sandy plugins', async () => { )!.instanceApi; expect(pluginInstance.connectedStub).toBeCalledTimes(1); expect(pluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(pluginInstance.activatedStub).toBeCalledTimes(1); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(0); // select non existing plugin act(() => { @@ -187,6 +195,8 @@ test('PluginContainer can render Sandy plugins', async () => { `); expect(pluginInstance.connectedStub).toBeCalledTimes(1); expect(pluginInstance.disconnectedStub).toBeCalledTimes(1); + expect(pluginInstance.activatedStub).toBeCalledTimes(1); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(1); // go back act(() => { @@ -199,6 +209,8 @@ test('PluginContainer can render Sandy plugins', async () => { }); expect(pluginInstance.connectedStub).toBeCalledTimes(2); expect(pluginInstance.disconnectedStub).toBeCalledTimes(1); + expect(pluginInstance.activatedStub).toBeCalledTimes(2); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(1); expect(client.rawSend).toBeCalledWith('init', {plugin: 'TestPlugin'}); // disable @@ -212,6 +224,8 @@ test('PluginContainer can render Sandy plugins', async () => { }); expect(pluginInstance.connectedStub).toBeCalledTimes(2); expect(pluginInstance.disconnectedStub).toBeCalledTimes(2); + expect(pluginInstance.activatedStub).toBeCalledTimes(2); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(2); expect(client.rawSend).toBeCalledWith('deinit', {plugin: 'TestPlugin'}); // re-enable @@ -226,6 +240,8 @@ test('PluginContainer can render Sandy plugins', async () => { // note: this is the old pluginInstance, so that one is not reconnected! expect(pluginInstance.connectedStub).toBeCalledTimes(2); expect(pluginInstance.disconnectedStub).toBeCalledTimes(2); + expect(pluginInstance.activatedStub).toBeCalledTimes(2); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(2); expect( client.sandyPluginStates.get('TestPlugin')!.instanceApi.connectedStub, @@ -233,6 +249,150 @@ test('PluginContainer can render Sandy plugins', async () => { expect(client.rawSend).toBeCalledWith('init', {plugin: 'TestPlugin'}); }); +test('PluginContainer triggers correct lifecycles for background plugin', async () => { + function MySandyPlugin() { + return
Hello from Sandy
; + } + + const plugin = (client: FlipperClient) => { + const connectedStub = jest.fn(); + const disconnectedStub = jest.fn(); + const activatedStub = jest.fn(); + const deactivatedStub = jest.fn(); + client.onConnect(connectedStub); + client.onDisconnect(disconnectedStub); + client.onActivate(activatedStub); + client.onDeactivate(deactivatedStub); + return {connectedStub, disconnectedStub, activatedStub, deactivatedStub}; + }; + + const definition = new SandyPluginDefinition( + TestUtils.createMockPluginDetails(), + { + plugin, + Component: MySandyPlugin, + }, + ); + const {act, client, store} = await renderMockFlipperWithPlugin(definition, { + onSend(method) { + if (method === 'getBackgroundPlugins') { + return {plugins: [definition.id]}; + } + }, + }); + + expect(client.rawSend).toBeCalledWith('init', {plugin: 'TestPlugin'}); + (client.rawSend as jest.Mock).mockClear(); + // make sure the plugin gets connected + const pluginInstance: ReturnType = client.sandyPluginStates.get( + definition.id, + )!.instanceApi; + expect(pluginInstance.connectedStub).toBeCalledTimes(1); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(pluginInstance.activatedStub).toBeCalledTimes(1); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(0); + + // select non existing plugin + act(() => { + store.dispatch( + selectPlugin({ + selectedPlugin: 'Logs', + deepLinkPayload: null, + }), + ); + }); + // bg plugin! + expect(client.rawSend).not.toBeCalled(); + (client.rawSend as jest.Mock).mockClear(); + expect(pluginInstance.connectedStub).toBeCalledTimes(1); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(pluginInstance.activatedStub).toBeCalledTimes(1); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(1); + + // go back + act(() => { + store.dispatch( + selectPlugin({ + selectedPlugin: definition.id, + deepLinkPayload: null, + }), + ); + }); + expect(pluginInstance.connectedStub).toBeCalledTimes(1); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(pluginInstance.activatedStub).toBeCalledTimes(2); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(1); + expect(client.rawSend).not.toBeCalled(); + (client.rawSend as jest.Mock).mockClear(); + + // disable + act(() => { + store.dispatch( + starPlugin({ + plugin: definition, + selectedApp: client.query.app, + }), + ); + }); + expect(pluginInstance.connectedStub).toBeCalledTimes(1); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(1); + expect(pluginInstance.activatedStub).toBeCalledTimes(2); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(2); + expect(client.rawSend).toBeCalledWith('deinit', {plugin: 'TestPlugin'}); + (client.rawSend as jest.Mock).mockClear(); + + // select something else + act(() => { + store.dispatch( + selectPlugin({ + selectedPlugin: 'Logs', + deepLinkPayload: null, + }), + ); + }); + // re-enable + act(() => { + store.dispatch( + starPlugin({ + plugin: definition, + selectedApp: client.query.app, + }), + ); + }); + // note: this is the old pluginInstance, so that one is not reconnected! + expect(pluginInstance.connectedStub).toBeCalledTimes(1); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(1); + expect(pluginInstance.activatedStub).toBeCalledTimes(2); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(2); + + const newPluginInstance: ReturnType = client.sandyPluginStates.get( + 'TestPlugin', + )!.instanceApi; + expect(newPluginInstance.connectedStub).toBeCalledTimes(1); + expect(newPluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(newPluginInstance.activatedStub).toBeCalledTimes(0); + expect(newPluginInstance.deactivatedStub).toBeCalledTimes(0); + expect(client.rawSend).toBeCalledWith('init', {plugin: 'TestPlugin'}); + (client.rawSend as jest.Mock).mockClear(); + + // select new plugin + act(() => { + store.dispatch( + selectPlugin({ + selectedPlugin: definition.id, + deepLinkPayload: null, + }), + ); + }); + + expect(newPluginInstance.connectedStub).toBeCalledTimes(1); + expect(newPluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(newPluginInstance.activatedStub).toBeCalledTimes(1); + expect(newPluginInstance.deactivatedStub).toBeCalledTimes(0); + expect(client.rawSend).not.toBeCalled(); + (client.rawSend as jest.Mock).mockClear(); +}); + test('PluginContainer + Sandy plugin supports deeplink', async () => { const linksSeen: any[] = []; diff --git a/desktop/app/src/index.tsx b/desktop/app/src/index.tsx index 0b70c5ccd..570215781 100644 --- a/desktop/app/src/index.tsx +++ b/desktop/app/src/index.tsx @@ -116,7 +116,10 @@ export {default as Orderable} from './ui/components/Orderable'; export {default as VirtualList} from './ui/components/VirtualList'; export {Component, PureComponent} from 'react'; export {default as ContextMenuProvider} from './ui/components/ContextMenuProvider'; -export {default as ContextMenu} from './ui/components/ContextMenu'; +export { + default as ContextMenu, + MenuTemplate, +} from './ui/components/ContextMenu'; export {FileListFile, FileListFiles} from './ui/components/FileList'; export {default as FileList} from './ui/components/FileList'; export {default as File} from './ui/components/File'; diff --git a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx index 3b5594aa1..a2b33492d 100644 --- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx @@ -206,13 +206,14 @@ type Renderer = RenderResult; export async function renderMockFlipperWithPlugin( pluginClazz: PluginDefinition, + options?: MockOptions, ): Promise< MockFlipperResult & { renderer: Renderer; act: (cb: () => void) => void; } > { - const args = await createMockFlipperWithPlugin(pluginClazz); + const args = await createMockFlipperWithPlugin(pluginClazz, options); function selectTestPlugin(store: Store, client: Client) { store.dispatch( diff --git a/desktop/flipper-plugin/src/__tests__/TestPlugin.tsx b/desktop/flipper-plugin/src/__tests__/TestPlugin.tsx index 314c0736c..32a0f49f3 100644 --- a/desktop/flipper-plugin/src/__tests__/TestPlugin.tsx +++ b/desktop/flipper-plugin/src/__tests__/TestPlugin.tsx @@ -25,6 +25,8 @@ type Methods = { export function plugin(client: FlipperClient) { const connectStub = jest.fn(); const disconnectStub = jest.fn(); + const activateStub = jest.fn(); + const deactivateStub = jest.fn(); const destroyStub = jest.fn(); const state = createState( { @@ -37,6 +39,8 @@ export function plugin(client: FlipperClient) { client.onConnect(connectStub); client.onDisconnect(disconnectStub); + client.onActivate(activateStub); + client.onDeactivate(deactivateStub); client.onDestroy(destroyStub); client.onMessage('inc', ({delta}) => { state.update((draft) => { @@ -64,6 +68,8 @@ export function plugin(client: FlipperClient) { } return { + activateStub, + deactivateStub, connectStub, destroyStub, disconnectStub, diff --git a/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx b/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx index 018dc056f..f56cbfdac 100644 --- a/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx +++ b/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx @@ -24,11 +24,15 @@ test('it can start a plugin and lifecycle events', () => { // startPlugin starts connected expect(instance.connectStub).toBeCalledTimes(1); expect(instance.disconnectStub).toBeCalledTimes(0); + expect(instance.activateStub).toBeCalledTimes(1); + expect(instance.deactivateStub).toBeCalledTimes(0); expect(instance.destroyStub).toBeCalledTimes(0); p.connect(); // noop expect(instance.connectStub).toBeCalledTimes(1); expect(instance.disconnectStub).toBeCalledTimes(0); + expect(instance.activateStub).toBeCalledTimes(1); + expect(instance.deactivateStub).toBeCalledTimes(0); expect(instance.destroyStub).toBeCalledTimes(0); p.disconnect(); @@ -38,15 +42,59 @@ test('it can start a plugin and lifecycle events', () => { expect(instance.disconnectStub).toBeCalledTimes(1); expect(instance.destroyStub).toBeCalledTimes(0); - p.destroy(); - expect(instance.connectStub).toBeCalledTimes(2); + p.deactivate(); // also disconnects + p.activate(); + expect(instance.connectStub).toBeCalledTimes(3); expect(instance.disconnectStub).toBeCalledTimes(2); + expect(instance.activateStub).toBeCalledTimes(2); + expect(instance.deactivateStub).toBeCalledTimes(1); + + p.destroy(); + expect(instance.connectStub).toBeCalledTimes(3); + expect(instance.disconnectStub).toBeCalledTimes(3); + expect(instance.activateStub).toBeCalledTimes(2); + expect(instance.deactivateStub).toBeCalledTimes(2); expect(instance.destroyStub).toBeCalledTimes(1); // cannot interact with destroyed plugin expect(() => { p.connect(); }).toThrowErrorMatchingInlineSnapshot(`"Plugin has been destroyed already"`); + expect(() => { + p.activate(); + }).toThrowErrorMatchingInlineSnapshot(`"Plugin has been destroyed already"`); +}); + +test('it can start a plugin and lifecycle events for background plugins', () => { + const {instance, ...p} = TestUtils.startPlugin(testPlugin, { + isBackgroundPlugin: true, + }); + + // @ts-expect-error + p.bla; + // @ts-expect-error + instance.bla; + + // startPlugin starts connected + expect(instance.connectStub).toBeCalledTimes(1); + expect(instance.disconnectStub).toBeCalledTimes(0); + expect(instance.activateStub).toBeCalledTimes(1); + expect(instance.deactivateStub).toBeCalledTimes(0); + expect(instance.destroyStub).toBeCalledTimes(0); + + p.deactivate(); // bg, no disconnection + p.activate(); + expect(instance.connectStub).toBeCalledTimes(1); + expect(instance.disconnectStub).toBeCalledTimes(0); + expect(instance.activateStub).toBeCalledTimes(2); + expect(instance.deactivateStub).toBeCalledTimes(1); + + p.destroy(); + expect(instance.connectStub).toBeCalledTimes(1); + expect(instance.disconnectStub).toBeCalledTimes(1); + expect(instance.activateStub).toBeCalledTimes(2); + expect(instance.deactivateStub).toBeCalledTimes(2); + expect(instance.destroyStub).toBeCalledTimes(1); }); test('it can render a plugin', () => { diff --git a/desktop/flipper-plugin/src/plugin/DevicePlugin.tsx b/desktop/flipper-plugin/src/plugin/DevicePlugin.tsx index 7315ace1a..70336869d 100644 --- a/desktop/flipper-plugin/src/plugin/DevicePlugin.tsx +++ b/desktop/flipper-plugin/src/plugin/DevicePlugin.tsx @@ -151,7 +151,10 @@ export class SandyDevicePluginInstance { } deactivate() { - if (!this.destroyed && this.activated) { + if (this.destroyed) { + return; + } + if (this.activated) { this.lastDeeplink = undefined; this.activated = false; this.events.emit('deactivate'); diff --git a/desktop/flipper-plugin/src/plugin/Plugin.tsx b/desktop/flipper-plugin/src/plugin/Plugin.tsx index 3304daf56..f7f967fca 100644 --- a/desktop/flipper-plugin/src/plugin/Plugin.tsx +++ b/desktop/flipper-plugin/src/plugin/Plugin.tsx @@ -32,6 +32,16 @@ export interface FlipperClient< */ onDestroy(cb: () => void): void; + /** + * the onActivate event is fired whenever the plugin is actived in the UI + */ + onActivate(cb: () => void): void; + + /** + * The counterpart of the `onActivate` handler. + */ + onDeactivate(cb: () => void): void; + /** * the onConnect event is fired whenever the plugin is connected to it's counter part on the device. * For most plugins this event is fired if the user selects the plugin, @@ -124,6 +134,7 @@ export class SandyPluginInstance { /** the plugin instance api as used inside components and such */ instanceApi: any; + activated = false; connected = false; destroyed = false; events = new EventEmitter(); @@ -146,6 +157,12 @@ export class SandyPluginInstance { onDestroy: (cb) => { this.events.on('destroy', cb); }, + onActivate: (cb) => { + this.events.on('activate', cb); + }, + onDeactivate: (cb) => { + this.events.on('deactivate', cb); + }, onConnect: (cb) => { this.events.on('connect', cb); }, @@ -181,22 +198,30 @@ export class SandyPluginInstance { // the plugin is selected in the UI activate() { this.assertNotDestroyed(); - const pluginId = this.definition.id; - if (!this.realClient.isBackgroundPlugin(pluginId)) { - this.realClient.initPlugin(pluginId); // will call connect() if needed + if (!this.activated) { + this.activated = true; + this.events.emit('activate'); + const pluginId = this.definition.id; + if (!this.realClient.isBackgroundPlugin(pluginId)) { + this.realClient.initPlugin(pluginId); // will call connect() if needed + } } } // the plugin is deselected in the UI deactivate() { - this.lastDeeplink = undefined; if (this.destroyed) { // this can happen if the plugin is disabled while active in the UI. // In that case deinit & destroy is already triggered from the STAR_PLUGIN action return; } + if (this.activated) { + this.lastDeeplink = undefined; + this.activated = false; + this.events.emit('deactivate'); + } const pluginId = this.definition.id; - if (!this.realClient.isBackgroundPlugin(pluginId)) { + if (this.connected && !this.realClient.isBackgroundPlugin(pluginId)) { this.realClient.deinitPlugin(pluginId); } } @@ -219,6 +244,7 @@ export class SandyPluginInstance { destroy() { this.assertNotDestroyed(); + this.deactivate(); if (this.connected) { this.realClient.deinitPlugin(this.definition.id); } diff --git a/desktop/flipper-plugin/src/test-utils/test-utils.tsx b/desktop/flipper-plugin/src/test-utils/test-utils.tsx index 0d17ed1bd..687a2e899 100644 --- a/desktop/flipper-plugin/src/test-utils/test-utils.tsx +++ b/desktop/flipper-plugin/src/test-utils/test-utils.tsx @@ -40,6 +40,7 @@ type Renderer = RenderResult; interface StartPluginOptions { initialState?: Record; isArchived?: boolean; + isBackgroundPlugin?: boolean; } type ExtractClientType> = Parameters< @@ -67,6 +68,15 @@ interface StartPluginResult> { * module, from which any other exposed methods can be accessed during testing */ module: Module; + /** + * Emulates the 'onActivate' event (when the user opens the plugin in the UI). + * Will also trigger the `onConnect` event for non-background plugins + */ + activate(): void; + /** + * Emulatese the 'onDeactivate' event + */ + deactivate(): void; /** * Emulates the 'onConnect' event */ @@ -127,7 +137,7 @@ interface StartDevicePluginResult { */ activate(): void; /** - * Emulatese the 'onDeactivate' event + * Emulates the 'onDeactivate' event */ deactivate(): void; /** @@ -164,15 +174,13 @@ export function startPlugin>( const sendStub = jest.fn(); const fakeFlipper: RealFlipperClient = { - isBackgroundPlugin(_pluginId: string) { - // we only reason about non-background plugins, - // as from testing perspective the difference shouldn't matter - return false; + isBackgroundPlugin() { + return !!options?.isBackgroundPlugin; }, - initPlugin(_pluginId: string) { + initPlugin() { pluginInstance.connect(); }, - deinitPlugin(_pluginId: string) { + deinitPlugin() { pluginInstance.disconnect(); }, call( @@ -190,12 +198,25 @@ export function startPlugin>( definition, options?.initialState, ); - // we start connected + if (options?.isBackgroundPlugin) { + pluginInstance.connect(); // otherwise part of activate + } + // we start activated pluginInstance.activate(); const res: StartPluginResult = { module, instance: pluginInstance.instanceApi, + activate() { + pluginInstance.activate(); + pluginInstance.connect(); + }, + deactivate() { + pluginInstance.deactivate(); + if (!fakeFlipper.isBackgroundPlugin) { + pluginInstance.disconnect(); + } + }, connect: () => pluginInstance.connect(), disconnect: () => pluginInstance.disconnect(), destroy: () => pluginInstance.destroy(), diff --git a/desktop/plugins/logs/index.tsx b/desktop/plugins/logs/index.tsx index c173e12b6..56b21dcf6 100644 --- a/desktop/plugins/logs/index.tsx +++ b/desktop/plugins/logs/index.tsx @@ -36,10 +36,10 @@ import { createPaste, textContent, KeyboardActions, + MenuTemplate, } from 'flipper'; import LogWatcher from './LogWatcher'; import React from 'react'; -import {MenuTemplate} from 'app/src/ui/components/ContextMenu'; const LOG_WATCHER_LOCAL_STORAGE_KEY = 'LOG_WATCHER_LOCAL_STORAGE_KEY';