From 489dd1521e349508a03023915f9c110cb15e1940 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 4 Aug 2020 07:05:57 -0700 Subject: [PATCH] Make sure Sandy Devices Plugins are loaded in Flipper devices Summary: This diff makes sure that devices will actually instantiate applicable sandy device plugins. Similar to how client plugins are owned by Client, device plugins are directly owned by BaseDevice, which significantly simplifies life cycle management and doesn't dispatch updates to all Redux connect components whenever something irrelevant changes. Also made sure `device.teardown()` is called. That API already existed, but wasn't used or implemented before. Updated Flipper test utils to support testing device plugins as well (both Sandy and classic ones) Reviewed By: passy, nikoant Differential Revision: D22693929 fbshipit-source-id: 73b2b8666ef7a0e748ea89360db84734d37eb5be --- desktop/app/src/Client.tsx | 8 +- desktop/app/src/devices/BaseDevice.tsx | 45 ++++++-- desktop/app/src/plugin.tsx | 8 +- .../__tests__/sandydeviceplugins.node.tsx | 107 ++++++++++++++++++ .../reducers/__tests__/sandyplugins.node.tsx | 2 - desktop/app/src/reducers/connections.tsx | 11 +- .../createMockFlipperWithPlugin.tsx | 11 +- desktop/app/src/utils/exportData.tsx | 5 +- desktop/app/src/utils/pluginUtils.tsx | 8 +- desktop/flipper-plugin/src/index.ts | 1 + desktop/plugins/crash_reporter/index.tsx | 7 +- 11 files changed, 180 insertions(+), 33 deletions(-) create mode 100644 desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index aff361228..8ced63073 100644 --- a/desktop/app/src/Client.tsx +++ b/desktop/app/src/Client.tsx @@ -68,12 +68,16 @@ const handleError = (store: Store, device: BaseDevice, error: ErrorType) => { if (isProduction()) { return; } - const crashReporterPlugin = store + const crashReporterPlugin: typeof FlipperDevicePlugin = store .getState() - .plugins.devicePlugins.get('CrashReporter'); + .plugins.devicePlugins.get('CrashReporter') as any; if (!crashReporterPlugin) { return; } + if (!crashReporterPlugin.persistedStateReducer) { + console.error('CrashReporterPlugin persistedStateReducer broken'); // Make sure we update this code if we ever convert it to Sandy + return; + } const pluginKey = getPluginKey(null, device, 'CrashReporter'); diff --git a/desktop/app/src/devices/BaseDevice.tsx b/desktop/app/src/devices/BaseDevice.tsx index 827893a21..42ac4f503 100644 --- a/desktop/app/src/devices/BaseDevice.tsx +++ b/desktop/app/src/devices/BaseDevice.tsx @@ -8,9 +8,14 @@ */ import stream from 'stream'; -import {FlipperDevicePlugin, DeviceLogListener} from 'flipper'; +import {DeviceLogListener} from 'flipper'; import {sortPluginsByName} from '../utils/pluginUtils'; -import {DeviceLogEntry} from 'flipper-plugin'; +import { + DeviceLogEntry, + SandyDevicePluginInstance, + SandyPluginDefinition, +} from 'flipper-plugin'; +import {DevicePluginMap} from '../plugin'; export type DeviceShell = { stdout: stream.Readable; @@ -64,7 +69,9 @@ export default class BaseDevice { source = ''; // sorted list of supported device plugins - devicePlugins!: string[]; + devicePlugins: string[] = []; + + sandyPluginStates = new Map(); supportsOS(os: OS) { return os.toLowerCase() === this.os.toLowerCase(); @@ -84,7 +91,11 @@ export default class BaseDevice { }; } - teardown() {} + teardown() { + for (const instance of this.sandyPluginStates.values()) { + instance.destroy(); + } + } supportedColumns(): Array { return ['date', 'pid', 'tid', 'tag', 'message', 'type', 'time']; @@ -158,10 +169,26 @@ export default class BaseDevice { return null; } - loadDevicePlugins(devicePlugins?: Map) { - this.devicePlugins = Array.from(devicePlugins ? devicePlugins.values() : []) - .filter((plugin) => plugin.supportsDevice(this)) - .sort(sortPluginsByName) - .map((plugin) => plugin.id); + loadDevicePlugins(devicePlugins?: DevicePluginMap) { + if (!devicePlugins) { + return; + } + const plugins = Array.from(devicePlugins.values()); + plugins.sort(sortPluginsByName); + for (const plugin of plugins) { + if (plugin instanceof SandyPluginDefinition) { + if (plugin.asDevicePluginModule().supportsDevice(this as any)) { + this.devicePlugins.push(plugin.id); + this.sandyPluginStates.set( + plugin.id, + new SandyDevicePluginInstance(this, plugin), + ); // TODO: pass initial state if applicable + } + } else { + if (plugin.supportsDevice(this)) { + this.devicePlugins.push(plugin.id); + } + } + } } } diff --git a/desktop/app/src/plugin.tsx b/desktop/app/src/plugin.tsx index 0e5d43a92..381cbd6ac 100644 --- a/desktop/app/src/plugin.tsx +++ b/desktop/app/src/plugin.tsx @@ -23,12 +23,14 @@ import {DEFAULT_MAX_QUEUE_SIZE} from './reducers/pluginMessageQueue'; import {PluginDetails} from 'flipper-plugin-lib'; import {Settings} from './reducers/settings'; import {SandyPluginDefinition} from 'flipper-plugin'; + type Parameters = {[key: string]: any}; export type PluginDefinition = ClientPluginDefinition | DevicePluginDefinition; -// TODO: T68738317 add SandyPluginDefinition -export type DevicePluginDefinition = typeof FlipperDevicePlugin; +export type DevicePluginDefinition = + | typeof FlipperDevicePlugin + | SandyPluginDefinition; export type ClientPluginDefinition = | typeof FlipperPlugin @@ -244,7 +246,7 @@ export class FlipperDevicePlugin< this.teardown(); } - static supportsDevice(_device: BaseDevice) { + static supportsDevice(_device: BaseDevice): boolean { throw new Error( 'supportsDevice is unimplemented in FlipperDevicePlugin class', ); diff --git a/desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx b/desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx new file mode 100644 index 000000000..7e64d90cf --- /dev/null +++ b/desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx @@ -0,0 +1,107 @@ +/** + * 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} from '../../'; +import {selectPlugin} from '../../reducers/connections'; +import { + SandyPluginDefinition, + SandyDevicePluginInstance, + DevicePluginClient, + TestUtils, +} from 'flipper-plugin'; + +interface PersistedState { + count: 1; +} + +const pluginDetails = TestUtils.createMockPluginDetails(); + +let initialized = false; + +beforeEach(() => { + initialized = false; +}); + +function devicePlugin(client: DevicePluginClient) { + const activateStub = jest.fn(); + const deactivateStub = jest.fn(); + const destroyStub = jest.fn(); + + client.onActivate(activateStub); + client.onDeactivate(deactivateStub); + client.onDestroy(destroyStub); + + initialized = true; + + return { + activateStub: activateStub, + deactivateStub: deactivateStub, + destroyStub, + }; +} +const TestPlugin = new SandyPluginDefinition(pluginDetails, { + supportsDevice: jest.fn().mockImplementation(() => true), + devicePlugin: jest + .fn() + .mockImplementation(devicePlugin) as typeof devicePlugin, + Component: jest.fn().mockImplementation(() => null), +}); + +type PluginApi = ReturnType; + +function selectTestPlugin(store: Store) { + store.dispatch( + selectPlugin({ + selectedPlugin: TestPlugin.id, + selectedApp: null, + deepLinkPayload: null, + selectedDevice: store.getState().connections.selectedDevice!, + }), + ); +} + +test('it should initialize device sandy plugins', async () => { + const {device, store} = await createMockFlipperWithPlugin(TestPlugin); + + // already started, so initialized immediately + expect(initialized).toBe(true); + expect(device.sandyPluginStates.get(TestPlugin.id)).toBeInstanceOf( + SandyDevicePluginInstance, + ); + expect(TestPlugin.asDevicePluginModule().supportsDevice).toBeCalledTimes(1); + const instanceApi: PluginApi = device.sandyPluginStates.get(TestPlugin.id)! + .instanceApi; + + expect(instanceApi.activateStub).toBeCalledTimes(0); + selectTestPlugin(store); + + // 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 + device.sandyPluginStates.get(TestPlugin.id)!.activate(); + expect(instanceApi.activateStub).toBeCalledTimes(1); + device.sandyPluginStates.get(TestPlugin.id)!.deactivate(); + expect(instanceApi.deactivateStub).toBeCalledTimes(1); + expect(instanceApi.destroyStub).toBeCalledTimes(0); +}); + +test('it should cleanup if device is removed', async () => { + const {device, store} = await createMockFlipperWithPlugin(TestPlugin); + const pluginInstance = device.sandyPluginStates.get(TestPlugin.id)!; + expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(0); + + // close device + store.dispatch({ + type: 'UNREGISTER_DEVICES', + payload: new Set([device.serial]), + }); + expect( + (pluginInstance.instanceApi as PluginApi).destroyStub, + ).toHaveBeenCalledTimes(1); +}); diff --git a/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx b/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx index b9355358d..36df06d30 100644 --- a/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx +++ b/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx @@ -233,5 +233,3 @@ test('it can send messages from sandy clients', async () => { } `); }); - -// TODO: T68683449 state is persisted if a plugin connects and reconnects diff --git a/desktop/app/src/reducers/connections.tsx b/desktop/app/src/reducers/connections.tsx index 919860a37..62ce3415a 100644 --- a/desktop/app/src/reducers/connections.tsx +++ b/desktop/app/src/reducers/connections.tsx @@ -232,9 +232,14 @@ export default (state: State = INITAL_STATE, action: Actions): State => { return updateSelection( produce(state, (draft) => { - draft.devices = draft.devices.filter( - (device) => !deviceSerials.has(device.serial), - ); + draft.devices = draft.devices.filter((device) => { + if (!deviceSerials.has(device.serial)) { + return true; + } else { + device.teardown(); + return false; + } + }); }), ); } diff --git a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx index 69d222c78..8ff9cdc9e 100644 --- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx @@ -34,7 +34,7 @@ import {Logger} from '../fb-interfaces/Logger'; import {PluginDefinition} from '../plugin'; import {registerPlugins} from '../reducers/plugins'; import PluginContainer from '../PluginContainer'; -import {getPluginKey} from '../utils/pluginUtils'; +import {getPluginKey, isDevicePluginDefinition} from '../utils/pluginUtils'; import {getInstance} from '../fb-stubs/Logger'; type MockFlipperResult = { @@ -75,6 +75,7 @@ export async function createMockFlipperWithPlugin( type: 'REGISTER_DEVICE', payload: device, }); + device.loadDevicePlugins(store.getState().plugins.devicePlugins); return device; } @@ -102,7 +103,7 @@ export async function createMockFlipperWithPlugin( null, // create a stub connection to avoid this plugin to be archived? logger, store, - [pluginClazz.id], + isDevicePluginDefinition(pluginClazz) ? [] : [pluginClazz.id], device, ); @@ -125,10 +126,7 @@ export async function createMockFlipperWithPlugin( case 'getPlugins': // assuming this plugin supports all plugins for now return { - plugins: [ - ...store.getState().plugins.clientPlugins.keys(), - ...store.getState().plugins.devicePlugins.keys(), - ], + plugins: [...store.getState().plugins.clientPlugins.keys()], }; case 'getBackgroundPlugins': return {plugins: []}; @@ -142,6 +140,7 @@ export async function createMockFlipperWithPlugin( // enable the plugin if ( + !isDevicePluginDefinition(pluginClazz) && !store .getState() .connections.userStarredPlugins[client.query.app]?.includes( diff --git a/desktop/app/src/utils/exportData.tsx b/desktop/app/src/utils/exportData.tsx index 98a0d97ed..d251477f8 100644 --- a/desktop/app/src/utils/exportData.tsx +++ b/desktop/app/src/utils/exportData.tsx @@ -19,7 +19,6 @@ import {PluginNotification} from '../reducers/notifications'; import Client, {ClientExport, ClientQuery} from '../Client'; import {pluginKey} from '../reducers/pluginStates'; import { - FlipperDevicePlugin, callClient, supportsMethod, PluginDefinition, @@ -82,7 +81,7 @@ type ProcessPluginStatesOptions = { clients: Array; serial: string; allPluginStates: PluginStatesState; - devicePlugins: Map; + devicePlugins: DevicePluginMap; selectedPlugins: Array; statusUpdate?: (msg: string) => void; }; @@ -91,7 +90,7 @@ type ProcessNotificationStatesOptions = { clients: Array; serial: string; allActiveNotifications: Array; - devicePlugins: Map; + devicePlugins: DevicePluginMap; statusUpdate?: (msg: string) => void; }; diff --git a/desktop/app/src/utils/pluginUtils.tsx b/desktop/app/src/utils/pluginUtils.tsx index ae0324a18..0c5495338 100644 --- a/desktop/app/src/utils/pluginUtils.tsx +++ b/desktop/app/src/utils/pluginUtils.tsx @@ -18,6 +18,7 @@ import {State as PluginStatesState} from '../reducers/pluginStates'; import {State as PluginsState} from '../reducers/plugins'; import {State as PluginMessageQueueState} from '../reducers/pluginMessageQueue'; import {deconstructPluginKey, deconstructClientId} from './clientUtils'; +import {SandyPluginDefinition} from 'flipper-plugin'; type Client = import('../Client').default; @@ -231,7 +232,8 @@ export function sortPluginsByName( export function isDevicePluginDefinition( definition: PluginDefinition, ): definition is DevicePluginDefinition { - // TODO: support Sandy device plugins T68738317 - // @ts-ignore - return definition.prototype instanceof FlipperDevicePlugin; + return ( + (definition as any).prototype instanceof FlipperDevicePlugin || + (definition instanceof SandyPluginDefinition && definition.isDevicePlugin) + ); } diff --git a/desktop/flipper-plugin/src/index.ts b/desktop/flipper-plugin/src/index.ts index 203d64c0f..cb9638494 100644 --- a/desktop/flipper-plugin/src/index.ts +++ b/desktop/flipper-plugin/src/index.ts @@ -16,6 +16,7 @@ export { DeviceLogListener, DevicePluginClient, LogLevel, + SandyDevicePluginInstance, } from './plugin/DevicePlugin'; export {SandyPluginDefinition} from './plugin/SandyPluginDefinition'; export {SandyPluginRenderer} from './plugin/PluginRenderer'; diff --git a/desktop/plugins/crash_reporter/index.tsx b/desktop/plugins/crash_reporter/index.tsx index bab95312f..344785ca0 100644 --- a/desktop/plugins/crash_reporter/index.tsx +++ b/desktop/plugins/crash_reporter/index.tsx @@ -19,7 +19,6 @@ import { ContextMenu, clipboard, Button, - FlipperPlugin, getPluginKey, getPersistedState, BaseDevice, @@ -239,10 +238,14 @@ export function parseCrashLogAndUpdateState( | typeof FlipperBasePlugin | undefined = store .getState() - .plugins.devicePlugins.get(CrashReporterPlugin.id); + .plugins.devicePlugins.get(CrashReporterPlugin.id) as any; if (!persistingPlugin) { return; } + if (!persistingPlugin.persistedStateReducer) { + console.error('CrashReporterPlugin is incompatible'); + return; + } const pluginStates = store.getState().pluginStates; const persistedState = getPersistedState( pluginKey,