From 699343a9cadbf460cf983e20eb502ff64bae2179 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 29 Apr 2021 12:12:00 -0700 Subject: [PATCH] Fix bug causing archived devices trying to connect Reviewed By: nikoant Differential Revision: D28064540 fbshipit-source-id: 43f05c4348a33e9633751bb9f69cd8d17ddd13c4 --- desktop/app/src/Client.tsx | 9 +- .../src/__tests__/PluginContainer.node.tsx | 259 ++++++++++++++++++ desktop/app/src/devices/ArchivedDevice.tsx | 4 +- desktop/app/src/test-utils/MockFlipper.tsx | 57 +++- .../createMockFlipperWithPlugin.tsx | 6 +- .../src/test-utils/test-utils.tsx | 6 + docs/extending/flipper-plugin.mdx | 2 + 7 files changed, 325 insertions(+), 18 deletions(-) diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index c119b6f61..2ab16344b 100644 --- a/desktop/app/src/Client.tsx +++ b/desktop/app/src/Client.tsx @@ -11,7 +11,6 @@ import {PluginDefinition, FlipperPlugin, FlipperDevicePlugin} from './plugin'; import BaseDevice, {OS} from './devices/BaseDevice'; import {Logger} from './fb-interfaces/Logger'; import {Store} from './reducers/index'; -import {setPluginState} from './reducers/pluginStates'; import {Payload, ConnectionStatus} from 'rsocket-types'; import {Flowable, Single} from 'rsocket-flowable'; import {performance} from 'perf_hooks'; @@ -150,7 +149,7 @@ export default class Client extends EventEmitter { device: BaseDevice, ) { super(); - this.connected.set(true); + this.connected.set(!!conn); this.plugins = plugins ? plugins : []; this.backgroundPlugins = []; this.connection = conn; @@ -696,8 +695,10 @@ export default class Client extends EventEmitter { initPlugin(pluginId: string) { this.activePlugins.add(pluginId); - this.rawSend('init', {plugin: pluginId}); - this.sandyPluginStates.get(pluginId)?.connect(); + if (this.connected.get()) { + this.rawSend('init', {plugin: pluginId}); + this.sandyPluginStates.get(pluginId)?.connect(); + } } deinitPlugin(pluginId: string) { diff --git a/desktop/app/src/__tests__/PluginContainer.node.tsx b/desktop/app/src/__tests__/PluginContainer.node.tsx index acd1f1ebb..c57013141 100644 --- a/desktop/app/src/__tests__/PluginContainer.node.tsx +++ b/desktop/app/src/__tests__/PluginContainer.node.tsx @@ -1015,3 +1015,262 @@ test('Sandy plugins support isPluginSupported + selectPlugin', async () => { `); expect(renders).toBe(2); }); + +test('PluginContainer can render Sandy plugins for archived devices', async () => { + let renders = 0; + + function MySandyPlugin() { + renders++; + const sandyApi = usePlugin(plugin); + const count = useValue(sandyApi.count); + expect(Object.keys(sandyApi)).toEqual([ + 'connectedStub', + 'disconnectedStub', + 'activatedStub', + 'deactivatedStub', + 'count', + ]); + expect(() => { + // eslint-disable-next-line + usePlugin(function bla() { + return {}; + }); + }).toThrowError(/didn't match the type of the requested plugin/); + return
Hello from Sandy{count}
; + } + + type Events = { + inc: {delta: number}; + }; + + const plugin = (client: PluginClient) => { + expect(client.connected.get()).toBeFalsy(); + expect(client.isConnected).toBeFalsy(); + expect(client.device.isConnected).toBeFalsy(); + expect(client.device.isArchived).toBeTruthy(); + const count = createState(0); + 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); + client.onMessage('inc', ({delta}) => { + count.set(count.get() + delta); + }); + return { + connectedStub, + disconnectedStub, + activatedStub, + deactivatedStub, + count, + }; + }; + + const definition = new _SandyPluginDefinition( + TestUtils.createMockPluginDetails(), + { + plugin, + Component: MySandyPlugin, + }, + ); + const { + renderer, + act, + client, + store, + } = await renderMockFlipperWithPlugin(definition, {archivedDevice: true}); + + expect(client.rawSend).not.toBeCalled(); + + expect(renderer.baseElement).toMatchInlineSnapshot(` + +
+
+
+ Hello from Sandy + 0 +
+
+
+
+ + `); + expect(renders).toBe(1); + + // make sure the plugin gets activated, but not connected! + const pluginInstance: ReturnType< + typeof plugin + > = client.sandyPluginStates.get(definition.id)!.instanceApi; + expect(pluginInstance.connectedStub).toBeCalledTimes(0); + 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, + }), + ); + }); + + expect(client.rawSend).not.toBeCalled(); + + expect(renderer.baseElement).toMatchInlineSnapshot(` + +
+ + `); + expect(pluginInstance.connectedStub).toBeCalledTimes(0); + 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, + }), + ); + }); + // Might be needed, but seems to work reliable without: await sleep(1000); + expect(renderer.baseElement).toMatchInlineSnapshot(` + +
+
+
+ Hello from Sandy + 0 +
+
+
+
+ + `); + + expect(pluginInstance.count.get()).toBe(0); + expect(pluginInstance.connectedStub).toBeCalledTimes(0); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(pluginInstance.activatedStub).toBeCalledTimes(2); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(1); + expect(client.rawSend).not.toBeCalled(); +}); + +test('PluginContainer triggers correct lifecycles for background plugin', async () => { + function MySandyPlugin() { + return
Hello from Sandy
; + } + + const plugin = (client: PluginClient) => { + expect(client.connected.get()).toBeFalsy(); + expect(client.isConnected).toBeFalsy(); + expect(client.device.isConnected).toBeFalsy(); + expect(client.device.isArchived).toBeTruthy(); + 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, { + archivedDevice: true, + onSend(_method) { + throw new Error('not to be called'); + }, + }); + + expect(client.rawSend).not.toBeCalled(); + // make sure the plugin gets connected + const pluginInstance: ReturnType< + typeof plugin + > = client.sandyPluginStates.get(definition.id)!.instanceApi; + expect(pluginInstance.connectedStub).toBeCalledTimes(0); + 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(); + expect(pluginInstance.connectedStub).toBeCalledTimes(0); + 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(0); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(pluginInstance.activatedStub).toBeCalledTimes(2); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(1); + expect(client.rawSend).not.toBeCalled(); + + // select something else + act(() => { + store.dispatch( + selectPlugin({ + selectedPlugin: 'Logs', + deepLinkPayload: null, + }), + ); + }); + // select new plugin + act(() => { + store.dispatch( + selectPlugin({ + selectedPlugin: definition.id, + deepLinkPayload: null, + }), + ); + }); + + expect(pluginInstance.connectedStub).toBeCalledTimes(0); + expect(pluginInstance.disconnectedStub).toBeCalledTimes(0); + expect(pluginInstance.activatedStub).toBeCalledTimes(3); + expect(pluginInstance.deactivatedStub).toBeCalledTimes(2); + expect(client.rawSend).not.toBeCalled(); +}); diff --git a/desktop/app/src/devices/ArchivedDevice.tsx b/desktop/app/src/devices/ArchivedDevice.tsx index 8be9d5449..c8ead36d7 100644 --- a/desktop/app/src/devices/ArchivedDevice.tsx +++ b/desktop/app/src/devices/ArchivedDevice.tsx @@ -20,7 +20,7 @@ export default class ArchivedDevice extends BaseDevice { deviceType: DeviceType; title: string; os: OS; - screenshotHandle: string | null; + screenshotHandle?: string | null; source?: string; supportRequestDetails?: SupportFormRequestDetailsState; }) { @@ -29,7 +29,7 @@ export default class ArchivedDevice extends BaseDevice { this.connected.set(false); this.source = options.source || ''; this.supportRequestDetails = options.supportRequestDetails; - this.archivedScreenshotHandle = options.screenshotHandle; + this.archivedScreenshotHandle = options.screenshotHandle ?? null; } archivedScreenshotHandle: string | null; diff --git a/desktop/app/src/test-utils/MockFlipper.tsx b/desktop/app/src/test-utils/MockFlipper.tsx index baa30ba3e..edb61684c 100644 --- a/desktop/app/src/test-utils/MockFlipper.tsx +++ b/desktop/app/src/test-utils/MockFlipper.tsx @@ -11,7 +11,7 @@ import {createStore} from 'redux'; import BaseDevice from '../devices/BaseDevice'; import {rootReducer} from '../store'; import {Store} from '../reducers/index'; -import Client, {ClientQuery} from '../Client'; +import Client, {ClientQuery, FlipperClientConnection} from '../Client'; import {buildClientId} from '../utils/clientUtils'; import {Logger} from '../fb-interfaces/Logger'; import {PluginDefinition} from '../plugin'; @@ -20,6 +20,7 @@ import {getInstance} from '../fb-stubs/Logger'; import {initializeFlipperLibImplementation} from '../utils/flipperLibImplementation'; import pluginManager from '../dispatcher/pluginManager'; import {PluginDetails} from 'flipper-plugin-lib'; +import ArchivedDevice from '../devices/ArchivedDevice'; export interface AppOptions { plugins?: PluginDefinition[]; @@ -37,6 +38,7 @@ export interface ClientOptions { export interface DeviceOptions { serial?: string; isSupportedByPlugin?: (p: PluginDetails) => boolean; + archived?: boolean; } export default class MockFlipper { @@ -110,13 +112,17 @@ export default class MockFlipper { public createDevice({ serial, isSupportedByPlugin, + archived, }: DeviceOptions = {}): BaseDevice { - const device = new BaseDevice( - serial ?? `serial_${++this._deviceCounter}`, - 'physical', - 'MockAndroidDevice', - 'Android', - ); + const s = serial ?? `serial_${++this._deviceCounter}`; + const device = archived + ? new ArchivedDevice({ + serial: s, + deviceType: 'emulator', + title: 'archived device', + os: 'Android', + }) + : new BaseDevice(s, 'physical', 'MockAndroidDevice', 'Android'); device.supportsPlugin = !isSupportedByPlugin ? () => true : isSupportedByPlugin; @@ -172,13 +178,12 @@ export default class MockFlipper { const client = new Client( id, query, - null, // create a stub connection to avoid this plugin to be archived? + device.isArchived ? null : createStubConnection(), this._logger, this._store, supportedPlugins, device, ); - // yikes client.device = { then() { @@ -212,7 +217,9 @@ export default class MockFlipper { }; client.rawSend = jest.fn(); - await client.init(); + if (!device.isArchived) { + await client.init(); + } // As convenience, by default we select the new client, star the plugin, and select it if (!skipRegister) { @@ -227,3 +234,33 @@ export default class MockFlipper { return client; } } +function createStubConnection(): + | FlipperClientConnection + | null + | undefined { + return { + close() { + throw new Error('Should not be called in test'); + }, + fireAndForget() { + throw new Error('Should not be called in test'); + }, + requestResponse() { + throw new Error('Should not be called in test'); + }, + connectionStatus() { + return { + subscribe() {}, + lift() { + throw new Error('Should not be called in test'); + }, + map() { + throw new Error('Should not be called in test'); + }, + take() { + throw new Error('Should not be called in test'); + }, + }; + }, + }; +} diff --git a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx index 1758cc214..46dfcba69 100644 --- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx @@ -62,6 +62,7 @@ type MockOptions = Partial<{ asBackgroundPlugin?: true; supportedPlugins?: string[]; device?: BaseDevice; + archivedDevice?: boolean; }>; function isPluginEnabled( @@ -90,7 +91,8 @@ export async function createMockFlipperWithPlugin( const logger = mockFlipper.logger; const store = mockFlipper.store; - const createDevice = (serial: string) => mockFlipper.createDevice({serial}); + const createDevice = (serial: string, archived?: boolean) => + mockFlipper.createDevice({serial, archived}); const createClient = async ( device: BaseDevice, name: string, @@ -131,7 +133,7 @@ export async function createMockFlipperWithPlugin( const device = options?.device ? mockFlipper.loadDevice(options?.device) - : createDevice('serial'); + : createDevice('serial', options?.archivedDevice); const client = await createClient(device, 'TestApp'); store.dispatch(selectDevice(device)); diff --git a/desktop/flipper-plugin/src/test-utils/test-utils.tsx b/desktop/flipper-plugin/src/test-utils/test-utils.tsx index dad9bf6be..05bcc9233 100644 --- a/desktop/flipper-plugin/src/test-utils/test-utils.tsx +++ b/desktop/flipper-plugin/src/test-utils/test-utils.tsx @@ -211,10 +211,16 @@ export function startPlugin>( }, connected: createState(true), initPlugin() { + if (options?.isArchived) { + return; + } this.connected.set(true); pluginInstance.connect(); }, deinitPlugin() { + if (options?.isArchived) { + return; + } this.connected.set(false); pluginInstance.disconnect(); }, diff --git a/docs/extending/flipper-plugin.mdx b/docs/extending/flipper-plugin.mdx index 641902a74..03a0c9112 100644 --- a/docs/extending/flipper-plugin.mdx +++ b/docs/extending/flipper-plugin.mdx @@ -111,6 +111,7 @@ This handler is untyped, and onMessage should be favored over using onUnhandledM Usage: `client.onActivate(callback: () => void)` Called when the plugin is selected by the user and mounted into the Flipper Desktop UI. See also the closely related `onConnect` event. +Unlike `onConnect`, `onActivate` will trigger as well for archived / imported devices. #### `onDeactivate` @@ -126,6 +127,7 @@ Usage: `client.onConnect(callback: () => void)` Triggered once the connection with the plugin on the client is established, and for example [`send`](#send) can be called safely. Typically, this happens when the plugin is activated (opened) in the Flipper Desktop. However, for [background plugins](create-plugin#background-plugins), this happens immediately after the plugin has been instantiated. +For archived / imported devices, this lifecycle is never triggered. #### `onDisconnect`