From bde112bf85a80bbec5ce41c3e93b50b5c723650f Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 1 Jul 2020 08:58:40 -0700 Subject: [PATCH] Introduce onConnect / onDisconnect hooks Summary: Introduced hooks that are called whenever the plugin is connected / disconnected to it's counter part on the device. There is some logic duplication between `PluginContainer` for old plugins, and `PluginRenderer` for new plugins, mostly caused by the fact that those lifecycles are triggered from the UI rather than from the reducers, but I figured refactoring that to be too risky. Reviewed By: jknoxville Differential Revision: D22232337 fbshipit-source-id: a384c45731a4c8d9b8b532a83e2becf49ce807c2 --- desktop/app/src/Client.tsx | 8 +- desktop/app/src/PluginContainer.tsx | 1 + .../src/__tests__/PluginContainer.node.tsx | 64 ++++++++-- .../createMockFlipperWithPlugin.node.tsx.snap | 1 + desktop/app/src/plugin.tsx | 5 +- .../reducers/__tests__/sandyplugins.node.tsx | 109 +++++++++++++----- .../createMockFlipperWithPlugin.tsx | 21 +++- desktop/flipper-plugin/src/plugin/Plugin.tsx | 55 ++++++++- .../src/plugin/PluginContext.tsx | 16 +-- .../src/plugin/PluginRenderer.tsx | 5 +- 10 files changed, 222 insertions(+), 63 deletions(-) diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index 302257710..dfccf9126 100644 --- a/desktop/app/src/Client.tsx +++ b/desktop/app/src/Client.tsx @@ -673,13 +673,15 @@ export default class Client extends EventEmitter { initPlugin(pluginId: string) { this.activePlugins.add(pluginId); this.rawSend('init', {plugin: pluginId}); - // TODO: call sandyOnConnect + this.sandyPluginStates.get(pluginId)?.connect(); } deinitPlugin(pluginId: string) { - // TODO: call sandyOnDisconnect this.activePlugins.delete(pluginId); - this.rawSend('deinit', {plugin: pluginId}); + this.sandyPluginStates.get(pluginId)?.disconnect(); + if (this.connected) { + this.rawSend('deinit', {plugin: pluginId}); + } } rawSend(method: string, params?: Object): void { diff --git a/desktop/app/src/PluginContainer.tsx b/desktop/app/src/PluginContainer.tsx index 34f8c181a..5026d1065 100644 --- a/desktop/app/src/PluginContainer.tsx +++ b/desktop/app/src/PluginContainer.tsx @@ -142,6 +142,7 @@ class PluginContainer extends PureComponent { | null | undefined, ) => { + // N.B. for Sandy plugins this lifecycle is managed by PluginRenderer if (this.plugin) { this.plugin._teardown(); this.plugin = null; diff --git a/desktop/app/src/__tests__/PluginContainer.node.tsx b/desktop/app/src/__tests__/PluginContainer.node.tsx index 1e6cbfb9a..4317ea481 100644 --- a/desktop/app/src/__tests__/PluginContainer.node.tsx +++ b/desktop/app/src/__tests__/PluginContainer.node.tsx @@ -14,7 +14,12 @@ import { renderMockFlipperWithPlugin, createMockPluginDetails, } from '../test-utils/createMockFlipperWithPlugin'; -import {SandyPluginContext, SandyPluginDefinition} from 'flipper-plugin'; +import { + SandyPluginContext, + SandyPluginDefinition, + FlipperClient, +} from 'flipper-plugin'; +import {selectPlugin} from '../reducers/connections'; interface PersistedState { count: 1; @@ -94,7 +99,13 @@ test('PluginContainer can render Sandy plugins', async () => { return
Hello from Sandy
; } - const plugin = () => ({}); + const plugin = (client: FlipperClient) => { + const connectedStub = jest.fn(); + const disconnectedStub = jest.fn(); + client.onConnect(connectedStub); + client.onDisconnect(disconnectedStub); + return {connectedStub, disconnectedStub}; + }; const definition = new SandyPluginDefinition(createMockPluginDetails(), { plugin, @@ -103,9 +114,13 @@ test('PluginContainer can render Sandy plugins', async () => { // any cast because this plugin is not enriched with the meta data that the plugin loader // normally adds. Our further sandy plugin test infra won't need this, but // for this test we do need to act a s a loaded plugin, to make sure PluginContainer itself can handle it - const {renderer, act, sendMessage} = await renderMockFlipperWithPlugin( - definition, - ); + const { + renderer, + act, + sendMessage, + client, + store, + } = await renderMockFlipperWithPlugin(definition); expect(renderer.baseElement).toMatchInlineSnapshot(`
@@ -129,9 +144,44 @@ test('PluginContainer can render Sandy plugins', async () => { act(() => { sendMessage('inc', {delta: 2}); }); + expect(renders).toBe(1); + + // 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); - // TODO: check that onConnect is called T68683507 // TODO: check that messages have arrived T68683442 - expect(renders).toBe(1); + // select non existing plugin + act(() => { + store.dispatch( + selectPlugin({ + selectedPlugin: 'Logs', + deepLinkPayload: null, + }), + ); + }); + + expect(renderer.baseElement).toMatchInlineSnapshot(` + +
+ + `); + expect(pluginInstance.connectedStub).toBeCalledTimes(1); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(1); + + // go back + act(() => { + store.dispatch( + selectPlugin({ + selectedPlugin: definition.id, + deepLinkPayload: null, + }), + ); + }); + expect(pluginInstance.connectedStub).toBeCalledTimes(2); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(1); }); diff --git a/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap b/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap index 7af97cf04..297ea342c 100644 --- a/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap +++ b/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap @@ -11,6 +11,7 @@ Object { "device": "MockAndroidDevice", "device_id": "serial", "os": "Android", + "sdk_version": 4, }, }, ], diff --git a/desktop/app/src/plugin.tsx b/desktop/app/src/plugin.tsx index fd8b64506..a2e5e4b85 100644 --- a/desktop/app/src/plugin.tsx +++ b/desktop/app/src/plugin.tsx @@ -297,10 +297,7 @@ export class FlipperPlugin< } // run plugin teardown this.teardown(); - if ( - this.realClient.connected && - !this.realClient.isBackgroundPlugin(pluginId) - ) { + if (!this.realClient.isBackgroundPlugin(pluginId)) { this.realClient.deinitPlugin(pluginId); } } diff --git a/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx b/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx index 83f841f66..f6ed69feb 100644 --- a/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx +++ b/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx @@ -26,24 +26,36 @@ interface PersistedState { const pluginDetails = createMockPluginDetails(); -let TestPlugin: SandyPluginDefinition; +let initialized = false; beforeEach(() => { - function plugin(client: FlipperClient) { - const destroyStub = jest.fn(); - - client.onDestroy(destroyStub); - - return { - destroyStub, - }; - } - TestPlugin = new SandyPluginDefinition(pluginDetails, { - plugin: jest.fn().mockImplementation(plugin) as typeof plugin, - Component: jest.fn().mockImplementation(() => null), - }); + initialized = false; }); +function plugin(client: FlipperClient) { + const connectStub = jest.fn(); + const disconnectStub = jest.fn(); + const destroyStub = jest.fn(); + + client.onConnect(connectStub); + client.onDisconnect(disconnectStub); + client.onDestroy(destroyStub); + + initialized = true; + + return { + connectStub, + disconnectStub, + destroyStub, + }; +} +const TestPlugin = new SandyPluginDefinition(pluginDetails, { + plugin: jest.fn().mockImplementation(plugin) as typeof plugin, + Component: jest.fn().mockImplementation(() => null), +}); + +type PluginApi = ReturnType; + function starTestPlugin(store: Store, client: Client) { store.dispatch( starPlugin({ @@ -68,27 +80,40 @@ 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(initialized).toBe(true); expect(client.sandyPluginStates.get(TestPlugin.id)).toBeInstanceOf( SandyPluginInstance, ); + const instanceApi: PluginApi = client.sandyPluginStates.get(TestPlugin.id)! + .instanceApi; + expect(instanceApi.connectStub).toBeCalledTimes(0); selectTestPlugin(store, client); - // TODO: make sure lifecycle 'activated' or something is triggered T68683507 + + // without rendering, non-bg plugins won't connect automatically, + // so this isn't the best test, but PluginContainer tests do test that part of the lifecycle + client.initPlugin(TestPlugin.id); + expect(instanceApi.connectStub).toBeCalledTimes(1); + client.deinitPlugin(TestPlugin.id); + expect(instanceApi.disconnectStub).toBeCalledTimes(1); + expect(instanceApi.destroyStub).toBeCalledTimes(0); }); test('it should cleanup a plugin if disabled', async () => { const {client, store} = await createMockFlipperWithPlugin(TestPlugin); expect(TestPlugin.module.plugin).toBeCalledTimes(1); - const pluginInstance = client.sandyPluginStates.get(TestPlugin.id)!; - expect(pluginInstance).toBeInstanceOf(SandyPluginInstance); - expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(0); + const pluginInstance: PluginApi = client.sandyPluginStates.get(TestPlugin.id)! + .instanceApi; + expect(pluginInstance.destroyStub).toHaveBeenCalledTimes(0); + client.initPlugin(TestPlugin.id); + expect(pluginInstance.connectStub).toHaveBeenCalledTimes(1); // unstar starTestPlugin(store, client); expect(client.sandyPluginStates.has(TestPlugin.id)).toBeFalsy(); - expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(1); + expect(pluginInstance.disconnectStub).toHaveBeenCalledTimes(1); + expect(pluginInstance.destroyStub).toHaveBeenCalledTimes(1); }); test('it should cleanup if client is removed', async () => { @@ -99,7 +124,9 @@ test('it should cleanup if client is removed', async () => { // close client client.close(); expect(client.sandyPluginStates.has(TestPlugin.id)).toBeFalsy(); - expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(1); + expect( + (pluginInstance.instanceApi as PluginApi).destroyStub, + ).toHaveBeenCalledTimes(1); }); test('it should not initialize a sandy plugin if not enabled', async () => { @@ -111,11 +138,7 @@ test('it should not initialize a sandy plugin if not enabled', async () => { id: 'Plugin2', }), { - plugin: jest.fn().mockImplementation((client) => { - const destroyStub = jest.fn(); - client.onDestroy(destroyStub); - return {destroyStub}; - }), + plugin: jest.fn().mockImplementation(plugin), Component() { return null; }, @@ -140,13 +163,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; + const instance = client.sandyPluginStates.get(Plugin2.id)! + .instanceApi as PluginApi; 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); + expect(instance.destroyStub).toHaveBeenCalledTimes(0); // disable plugin again store.dispatch( @@ -157,7 +180,33 @@ test('it should not initialize a sandy plugin if not enabled', async () => { ); expect(client.sandyPluginStates.get(Plugin2.id)).toBeUndefined(); - expect(destroyStub).toHaveBeenCalledTimes(1); + expect(instance.connectStub).toHaveBeenCalledTimes(0); + // disconnect wasn't called because connect was never called + expect(instance.disconnectStub).toHaveBeenCalledTimes(0); + expect(instance.destroyStub).toHaveBeenCalledTimes(1); +}); + +test('it trigger hooks for background plugins', async () => { + const {client} = await createMockFlipperWithPlugin(TestPlugin, { + onSend(method) { + if (method === 'getBackgroundPlugins') { + return {plugins: [TestPlugin.id]}; + } + }, + }); + const pluginInstance: PluginApi = client.sandyPluginStates.get(TestPlugin.id)! + .instanceApi; + expect(client.isBackgroundPlugin(TestPlugin.id)).toBeTruthy(); + expect(pluginInstance.destroyStub).toHaveBeenCalledTimes(0); + expect(pluginInstance.connectStub).toHaveBeenCalledTimes(1); + expect(pluginInstance.disconnectStub).toHaveBeenCalledTimes(0); + + // close client + client.close(); + expect(client.sandyPluginStates.has(TestPlugin.id)).toBeFalsy(); + expect(pluginInstance.destroyStub).toHaveBeenCalledTimes(1); + expect(pluginInstance.connectStub).toHaveBeenCalledTimes(1); + expect(pluginInstance.disconnectStub).toHaveBeenCalledTimes(1); }); // TODO: T68683449 state is persisted if a plugin connects and reconnects diff --git a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx index 229010624..ecbe07d11 100644 --- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx @@ -47,8 +47,17 @@ type MockFlipperResult = { logger: Logger; }; +type MockOptions = Partial<{ + /** + * can be used to intercept outgoing calls. If it returns undefined + * the base implementation will be used + */ + onSend(method: string, params?: object): object | undefined; +}>; + export async function createMockFlipperWithPlugin( pluginClazz: PluginDefinition, + options?: MockOptions, ): Promise { const store = createStore(reducers); const logger = getInstance(); @@ -77,6 +86,7 @@ export async function createMockFlipperWithPlugin( os: 'Android', device: device.title, device_id: device.serial, + sdk_version: 4, }; const id = buildClientId({ app: query.app, @@ -101,8 +111,11 @@ export async function createMockFlipperWithPlugin( return device; }, } as any; - client.rawCall = async (method, _fromPlugin, _params): Promise => { - // TODO: could use an interceptor here + client.rawCall = async (method, _fromPlugin, params): Promise => { + const intercepted = options?.onSend?.(method, params); + if (intercepted !== undefined) { + return intercepted; + } switch (method) { case 'getPlugins': // assuming this plugin supports all plugins for now @@ -112,8 +125,10 @@ export async function createMockFlipperWithPlugin( ...store.getState().plugins.devicePlugins.keys(), ], }; + case 'getBackgroundPlugins': + return {plugins: []}; default: - throw new Error(`Test client doesn't supoprt rawCall to ${method}`); + throw new Error(`Test client doesn't support rawCall to ${method}`); } }; diff --git a/desktop/flipper-plugin/src/plugin/Plugin.tsx b/desktop/flipper-plugin/src/plugin/Plugin.tsx index 1f401b6d4..e1e5de4d7 100644 --- a/desktop/flipper-plugin/src/plugin/Plugin.tsx +++ b/desktop/flipper-plugin/src/plugin/Plugin.tsx @@ -24,13 +24,32 @@ export interface FlipperClient< * the onDestroy event is fired whenever a client is unloaded from Flipper, or a plugin is disabled. */ onDestroy(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, + * for background plugins when the initial connection is made. + */ + onConnect(cb: () => void): void; + + /** + * The counterpart of the `onConnect` handler. + * Will also be fired before the plugin is cleaned up if the connection is currently active: + * - when the client disconnects + * - when the plugin is disabled + */ + onDisconnect(cb: () => void): void; } /** * Internal API exposed by Flipper, and wrapped by FlipperPluginInstance to be passed to the * Plugin Factory. For internal purposes only */ -interface RealFlipperClient {} +interface RealFlipperClient { + isBackgroundPlugin(pluginId: string): boolean; + initPlugin(pluginId: string): void; + deinitPlugin(pluginId: string): void; +} export type FlipperPluginFactory< Events extends EventsContract, @@ -49,6 +68,7 @@ export class SandyPluginInstance { /** the plugin instance api as used inside components and such */ instanceApi: any; + connected = false; events = new EventEmitter(); constructor( @@ -61,19 +81,48 @@ export class SandyPluginInstance { onDestroy: (cb) => { this.events.on('destroy', cb); }, + onConnect: (cb) => { + this.events.on('connect', cb); + }, + onDisconnect: (cb) => { + this.events.on('disconnect', cb); + }, }; this.instanceApi = definition.module.plugin(this.client); } + // the plugin is selected in the UI activate() { - // TODO: T68683507 + 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() { - // TODO: T68683507 + const pluginId = this.definition.id; + if (!this.realClient.isBackgroundPlugin(pluginId)) { + this.realClient.deinitPlugin(pluginId); + } + } + + connect() { + if (!this.connected) { + this.connected = true; + this.events.emit('connect'); + } + } + + disconnect() { + if (this.connected) { + this.connected = false; + this.events.emit('disconnect'); + } } destroy() { + this.disconnect(); this.events.emit('destroy'); } diff --git a/desktop/flipper-plugin/src/plugin/PluginContext.tsx b/desktop/flipper-plugin/src/plugin/PluginContext.tsx index 01e9be2f7..49cdca3ad 100644 --- a/desktop/flipper-plugin/src/plugin/PluginContext.tsx +++ b/desktop/flipper-plugin/src/plugin/PluginContext.tsx @@ -8,16 +8,8 @@ */ import {createContext} from 'react'; +import {SandyPluginInstance} from './Plugin'; -export type SandyPluginContext = { - deactivate(): void; -}; - -// TODO: to be filled in later with testing and such -const stubPluginContext: SandyPluginContext = { - deactivate() {}, -}; - -export const SandyPluginContext = createContext( - stubPluginContext, -); +export const SandyPluginContext = createContext< + SandyPluginInstance | undefined +>(undefined); diff --git a/desktop/flipper-plugin/src/plugin/PluginRenderer.tsx b/desktop/flipper-plugin/src/plugin/PluginRenderer.tsx index dd72e8434..8880e03cd 100644 --- a/desktop/flipper-plugin/src/plugin/PluginRenderer.tsx +++ b/desktop/flipper-plugin/src/plugin/PluginRenderer.tsx @@ -21,7 +21,10 @@ type Props = { export const SandyPluginRenderer = memo( ({plugin}: Props) => { useEffect(() => { - plugin.deactivate(); + plugin.activate(); + return () => { + plugin.deactivate(); + }; }, [plugin]); return (