From 1dc9e899b876a4c42e06e931addc75dafbc7e6af Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 1 Jul 2020 08:58:40 -0700 Subject: [PATCH] Make sure Sandy plugis can be initialized Summary: This diff makes sure sandy plugins are initialized. Sandy plugins are stored directly in the client for two reasons 1. we want to decouple any plugin state updates from our central redux store, as redux is particularly bad in handling high frequency updates. 2. The lifecycle of a plugin is now bound to the lifecycle of a client. This means that we don't need 'persistedStore' to make sure state is preserved; that is now the default. Switching plugins will no longer reinitialize them (but only run specific hooks, see later diffs). 3. PersistedState will be introduced for sandy plugins as well later, but primarily for import / export / debug reasons. A significant difference with the current persistent state, is that if a client crashes and reconnects, plugins will loose their state. We can prevent this (again, since state persisting will be reintroduced), but I'm not sure we need that for the specific reconnect scenario. Because 1. we should fix reconnecting clients anyway, and from stats it looks to happen less already 2. reconnects are usually caused by plugins that aggregate a lot of data and get slower over time. Restoring old state also restores those unstabilites. For the overview bringing back the archi picture of earlier diff: {F241508042} Also fixed a bug where enabling background plugins didn't enable them on all devices with that app. Reviewed By: jknoxville Differential Revision: D22186276 fbshipit-source-id: 3fd42b577f86920e5280aa8cce1a0bc4d5564ed9 --- desktop/app/src/Client.tsx | 63 +++++++- desktop/app/src/PluginContainer.tsx | 2 +- .../src/chrome/mainsidebar/MainSidebar2.tsx | 8 +- .../reducers/__tests__/sandyplugins.node.tsx | 141 ++++++++++++++++++ desktop/app/src/reducers/connections.tsx | 41 +++-- desktop/app/src/reducers/supportForm.tsx | 5 +- desktop/app/src/server.tsx | 2 +- .../createMockFlipperWithPlugin.tsx | 72 ++++++--- .../src/utils/__tests__/messageQueue.node.tsx | 44 ++++-- desktop/flipper-plugin/src/plugin/Plugin.tsx | 25 +++- 10 files changed, 333 insertions(+), 70 deletions(-) create mode 100644 desktop/app/src/reducers/__tests__/sandyplugins.node.tsx diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index 122efd522..65de1bd0e 100644 --- a/desktop/app/src/Client.tsx +++ b/desktop/app/src/Client.tsx @@ -32,7 +32,7 @@ import {sideEffect} from './utils/sideEffect'; import {emitBytesReceived} from './dispatcher/tracking'; import {debounce} from 'lodash'; import {batch} from 'react-redux'; -import {SandyPluginDefinition} from 'flipper-plugin'; +import {SandyPluginDefinition, SandyPluginInstance} from 'flipper-plugin'; type Plugins = Array; @@ -135,6 +135,7 @@ export default class Client extends EventEmitter { messages: Params[]; } > = {}; + sandyPluginStates = new Map(); requestCallbacks: Map< number, @@ -253,18 +254,26 @@ export default class Client extends EventEmitter { return this.backgroundPlugins.includes(pluginId); } + isEnabledPlugin(pluginId: string) { + return this.store + .getState() + .connections.userStarredPlugins[this.query.app]?.includes(pluginId); + } + shouldConnectAsBackgroundPlugin(pluginId: string) { return ( defaultEnabledBackgroundPlugins.includes(pluginId) || - this.store - .getState() - .connections.userStarredPlugins[this.query.app]?.includes(pluginId) + this.isEnabledPlugin(pluginId) ); } async init() { this.setMatchingDevice(); await this.loadPlugins(); + // this starts all sandy enabled plugin + this.plugins.forEach((pluginId) => + this.startPluginIfNeeded(this.getPlugin(pluginId)), + ); this.backgroundPlugins = await this.getBackgroundPlugins(); this.backgroundPlugins.forEach((plugin) => { if (this.shouldConnectAsBackgroundPlugin(plugin)) { @@ -298,6 +307,47 @@ export default class Client extends EventEmitter { return plugins; } + startPluginIfNeeded( + plugin: PluginDefinition | undefined, + isEnabled = plugin ? this.isEnabledPlugin(plugin.id) : false, + ) { + // start a plugin on start if it is a SandyPlugin, which is starred, and doesn't have persisted state yet + if ( + plugin instanceof SandyPluginDefinition && + isEnabled && + !this.sandyPluginStates.has(plugin.id) + ) { + // TODO: needs to be wrapped in error tracking T68955280 + // TODO: pick up any existing persisted state T68683449 + this.sandyPluginStates.set( + plugin.id, + new SandyPluginInstance(this, plugin), + ); + } + } + + stopPluginIfNeeded(pluginId: string) { + const instance = this.sandyPluginStates.get(pluginId); + if (instance) { + instance.destroy(); + // TODO: make sure persisted state is writtenT68683449 + this.sandyPluginStates.delete(pluginId); + } + } + + close() { + this.emit('close'); + this.plugins.forEach((pluginId) => this.stopPluginIfNeeded(pluginId)); + } + + // gets a plugin by pluginId + getPlugin(pluginId: string): PluginDefinition | undefined { + const plugins = this.store.getState().plugins; + return ( + plugins.clientPlugins.get(pluginId) || plugins.devicePlugins.get(pluginId) + ); + } + // get the supported background plugins async getBackgroundPlugins(): Promise { if (this.sdkVersion < 4) { @@ -313,6 +363,9 @@ export default class Client extends EventEmitter { async refreshPlugins() { const oldBackgroundPlugins = this.backgroundPlugins; await this.loadPlugins(); + this.plugins.forEach((pluginId) => + this.startPluginIfNeeded(this.getPlugin(pluginId)), + ); const newBackgroundPlugins = await this.getBackgroundPlugins(); this.backgroundPlugins = newBackgroundPlugins; // diff the background plugin list, disconnect old, connect new ones @@ -616,9 +669,11 @@ export default class Client extends EventEmitter { initPlugin(pluginId: string) { this.activePlugins.add(pluginId); this.rawSend('init', {plugin: pluginId}); + // TODO: call sandyOnConnect } deinitPlugin(pluginId: string) { + // TODO: call sandyOnDisconnect this.activePlugins.delete(pluginId); this.rawSend('deinit', {plugin: pluginId}); } diff --git a/desktop/app/src/PluginContainer.tsx b/desktop/app/src/PluginContainer.tsx index 765611996..82fdd275b 100644 --- a/desktop/app/src/PluginContainer.tsx +++ b/desktop/app/src/PluginContainer.tsx @@ -274,7 +274,7 @@ class PluginContainer extends PureComponent { toggled={false} onClick={() => { this.props.starPlugin({ - selectedPlugin: activePlugin.id, + plugin: activePlugin, selectedApp: (this.props.target as Client).query.app, }); }} diff --git a/desktop/app/src/chrome/mainsidebar/MainSidebar2.tsx b/desktop/app/src/chrome/mainsidebar/MainSidebar2.tsx index 510893960..053c8bc44 100644 --- a/desktop/app/src/chrome/mainsidebar/MainSidebar2.tsx +++ b/desktop/app/src/chrome/mainsidebar/MainSidebar2.tsx @@ -486,10 +486,10 @@ const PluginList = memo(function PluginList({ }, [client]); const onFavorite = useCallback( - (plugin: string) => { + (plugin: PluginDefinition) => { starPlugin({ selectedApp: client.query.app, - selectedPlugin: plugin, + plugin, }); }, [client], @@ -589,7 +589,7 @@ const PluginsByCategory = memo(function PluginsByCategory({ starred: boolean; selectedPlugin?: null | string; selectedApp?: null | string; - onFavorite: (pluginId: string) => void; + onFavorite: (plugin: PluginDefinition) => void; selectPlugin: SelectPlugin; }) { return ( @@ -617,7 +617,7 @@ const PluginsByCategory = memo(function PluginsByCategory({ } plugin={plugin} app={client.query.app} - onFavorite={() => onFavorite(plugin.id)} + onFavorite={() => onFavorite(plugin)} starred={device.isArchived ? undefined : starred} /> ))} diff --git a/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx b/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx new file mode 100644 index 000000000..9cc5c2a5d --- /dev/null +++ b/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx @@ -0,0 +1,141 @@ +/** + * 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, Client} from '../../'; +import {selectPlugin, starPlugin} from '../../reducers/connections'; +import {registerPlugins} from '../../reducers/plugins'; +import {SandyPluginDefinition, SandyPluginInstance} from 'flipper-plugin'; + +interface PersistedState { + count: 1; +} + +const pluginDetails = { + id: 'TestPlugin', + dir: '', + name: 'TestPlugin', + specVersion: 0, + entry: '', + isDefault: false, + main: '', + source: '', + title: 'Testing Plugin', + version: '', +} as const; + +let TestPlugin: SandyPluginDefinition; + +beforeEach(() => { + TestPlugin = new SandyPluginDefinition(pluginDetails, { + plugin: jest.fn().mockImplementation(() => ({})), + Component: jest.fn().mockImplementation(() => null), + }); +}); + +function starTestPlugin(store: Store, client: Client) { + store.dispatch( + starPlugin({ + plugin: TestPlugin, + selectedApp: client.query.app, + }), + ); +} + +function selectTestPlugin(store: Store, client: Client) { + store.dispatch( + selectPlugin({ + selectedPlugin: TestPlugin.id, + selectedApp: client.query.app, + deepLinkPayload: null, + selectedDevice: store.getState().connections.selectedDevice!, + }), + ); +} + +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(client.sandyPluginStates.get(TestPlugin.id)).toBeInstanceOf( + SandyPluginInstance, + ); + + selectTestPlugin(store, client); + // TODO: make sure lifecycle 'activated' or something is triggered T68683507 +}); + +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( + SandyPluginInstance, + ); + + // unstar + starTestPlugin(store, client); + expect(client.sandyPluginStates.get(TestPlugin.id)).toBeUndefined(); + // TODO: make sure onDestroy is called T68683507 +}); + +test('it should not initialize a sandy plugin if not enabled', async () => { + const {client, store} = await createMockFlipperWithPlugin(TestPlugin); + + const Plugin2 = new SandyPluginDefinition( + { + ...pluginDetails, + name: 'Plugin2', + id: 'Plugin2', + }, + { + plugin: jest.fn().mockImplementation(() => ({})), + Component() { + return null; + }, + }, + ); + + const pluginState1 = client.sandyPluginStates.get(TestPlugin.id); + expect(pluginState1).toBeInstanceOf(SandyPluginInstance); + store.dispatch(registerPlugins([Plugin2])); + await client.refreshPlugins(); + // not yet enabled, so not yet started + expect(client.sandyPluginStates.get(Plugin2.id)).toBeUndefined(); + expect(Plugin2.module.plugin).toBeCalledTimes(0); + + store.dispatch( + starPlugin({ + plugin: Plugin2, + selectedApp: client.query.app, + }), + ); + + expect(client.sandyPluginStates.get(Plugin2.id)).toBeInstanceOf( + SandyPluginInstance, + ); + expect(client.sandyPluginStates.get(TestPlugin.id)).toBe(pluginState1); // not reinitialized + + expect(TestPlugin.module.plugin).toBeCalledTimes(1); + expect(Plugin2.module.plugin).toBeCalledTimes(1); + + // disable plugin again + store.dispatch( + starPlugin({ + plugin: Plugin2, + selectedApp: client.query.app, + }), + ); + + expect(client.sandyPluginStates.get(Plugin2.id)).toBeUndefined(); + // TODO: test destroy hook is called T68683507 +}); + +// 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 e9d6a7980..8398f736f 100644 --- a/desktop/app/src/reducers/connections.tsx +++ b/desktop/app/src/reducers/connections.tsx @@ -28,7 +28,7 @@ import { defaultEnabledBackgroundPlugins, } from '../utils/pluginUtils'; import {deconstructClientId} from '../utils/clientUtils'; -import {FlipperDevicePlugin} from '../plugin'; +import {FlipperDevicePlugin, PluginDefinition} from '../plugin'; import {RegisterPluginAction} from './plugins'; import {SandyPluginDefinition} from 'flipper-plugin'; @@ -138,8 +138,8 @@ export type Action = | { type: 'STAR_PLUGIN'; payload: { - selectedPlugin: string; selectedApp: string; + plugin: PluginDefinition; }; } | { @@ -279,33 +279,46 @@ export default (state: State = INITAL_STATE, action: Actions): State => { } case 'STAR_PLUGIN': { - const {selectedPlugin, selectedApp} = action.payload; - const client = state.clients.find( + const {plugin, selectedApp} = action.payload; + const selectedPlugin = plugin.id; + const clients = state.clients.filter( (client) => client.query.app === selectedApp, ); return produce(state, (draft) => { if (!draft.userStarredPlugins[selectedApp]) { - draft.userStarredPlugins[selectedApp] = [selectedPlugin]; - } else { - const plugins = draft.userStarredPlugins[selectedApp]; - const idx = plugins.indexOf(selectedPlugin); - if (idx === -1) { - plugins.push(selectedPlugin); + draft.userStarredPlugins[selectedApp] = []; + } + const plugins = draft.userStarredPlugins[selectedApp]; + const idx = plugins.indexOf(selectedPlugin); + if (idx === -1) { + plugins.push(selectedPlugin); + // enabling a plugin on one device enables it on all... + clients.forEach((client) => { + // sandy plugin? initialize it + client.startPluginIfNeeded(plugin, true); + // background plugin? connect it needed if ( !defaultEnabledBackgroundPlugins.includes(selectedPlugin) && client?.isBackgroundPlugin(selectedPlugin) ) { client.initPlugin(selectedPlugin); } - } else { - plugins.splice(idx, 1); + }); + } else { + plugins.splice(idx, 1); + // enabling a plugin on one device disables it on all... + clients.forEach((client) => { + // disconnect background plugins if ( !defaultEnabledBackgroundPlugins.includes(selectedPlugin) && client?.isBackgroundPlugin(selectedPlugin) ) { client.deinitPlugin(selectedPlugin); } - } + // stop sandy plugins + // TODO: forget any persisted state as well T68683449 + client.stopPluginIfNeeded(plugin.id); + }); } }); } @@ -500,7 +513,7 @@ export const selectPlugin = (payload: { }); export const starPlugin = (payload: { - selectedPlugin: string; + plugin: PluginDefinition; selectedApp: string; }): Action => ({ type: 'STAR_PLUGIN', diff --git a/desktop/app/src/reducers/supportForm.tsx b/desktop/app/src/reducers/supportForm.tsx index 4ef7be6c8..56bb9a9c0 100644 --- a/desktop/app/src/reducers/supportForm.tsx +++ b/desktop/app/src/reducers/supportForm.tsx @@ -134,10 +134,13 @@ export class Group { selectedClient.plugins.includes(requiredPlugin) && !requiredPluginEnabled ) { + const plugin = + store.getState().plugins.clientPlugins.get(requiredPlugin) || + store.getState().plugins.devicePlugins.get(requiredPlugin)!; store.dispatch( setStarPlugin({ selectedApp: app, - selectedPlugin: requiredPlugin, + plugin, }), ); } else if ( diff --git a/desktop/app/src/server.tsx b/desktop/app/src/server.tsx index 90431bf08..64a4b776c 100644 --- a/desktop/app/src/server.tsx +++ b/desktop/app/src/server.tsx @@ -507,7 +507,7 @@ class Server extends EventEmitter { removeConnection(id: string) { const info = this.connections.get(id); if (info) { - info.client.emit('close'); + info.client.close(); this.connections.delete(id); this.emit('clients-change'); this.emit('removed-client', id); diff --git a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx index c24d322db..cb25754a8 100644 --- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx @@ -29,33 +29,28 @@ import Client, {ClientQuery} from '../Client'; import {buildClientId} from '../utils/clientUtils'; import {Logger} from '../fb-interfaces/Logger'; -import {FlipperPlugin} from '../plugin'; +import {FlipperPlugin, PluginDefinition} from '../plugin'; import {registerPlugins} from '../reducers/plugins'; import PluginContainer from '../PluginContainer'; - -export function createStubLogger(): Logger { - return { - ...console, - track: console.info, - trackTimeSince: console.info, - }; -} +import {getPluginKey} from '../utils/pluginUtils'; +import {getInstance} from '../fb-stubs/Logger'; type MockFlipperResult = { client: Client; device: BaseDevice; store: Store; - sendMessage(method: string, params: any): void; + pluginKey: string; + sendMessage(method: string, params: any, client?: Client): void; createDevice(serial: string): BaseDevice; - createClient(device: BaseDevice, name: string): Client; + createClient(device: BaseDevice, name: string): Promise; logger: Logger; }; export async function createMockFlipperWithPlugin( - pluginClazz: typeof FlipperPlugin, + pluginClazz: PluginDefinition, ): Promise { const store = createStore(reducers); - const logger = createStubLogger(); + const logger = getInstance(); store.dispatch(registerPlugins([pluginClazz])); function createDevice(serial: string): BaseDevice { @@ -72,7 +67,10 @@ export async function createMockFlipperWithPlugin( return device; } - function createClient(device: BaseDevice, name: string): Client { + async function createClient( + device: BaseDevice, + name: string, + ): Promise { const query: ClientQuery = { app: name, os: 'Android', @@ -102,6 +100,38 @@ export async function createMockFlipperWithPlugin( return device; }, } as any; + client.rawCall = async (method, _fromPlugin, _params): Promise => { + // TODO: could use an interceptor here + switch (method) { + case 'getPlugins': + // assuming this plugin supports all plugins for now + return { + plugins: [ + ...store.getState().plugins.clientPlugins.keys(), + ...store.getState().plugins.devicePlugins.keys(), + ], + }; + default: + throw new Error(`Test client doesn't supoprt rawCall to ${method}`); + } + }; + + // enable the plugin + if ( + !store + .getState() + .connections.userStarredPlugins[client.query.app]?.includes( + pluginClazz.id, + ) + ) { + store.dispatch( + starPlugin({ + plugin: pluginClazz, + selectedApp: client.query.app, + }), + ); + } + await client.init(); // As convenience, by default we select the new client, star the plugin, and select it store.dispatch({ @@ -112,18 +142,11 @@ export async function createMockFlipperWithPlugin( } const device = createDevice('serial'); - const client = createClient(device, 'TestApp'); + const client = await createClient(device, 'TestApp'); store.dispatch(selectDevice(device)); store.dispatch(selectClient(client.id)); - store.dispatch( - starPlugin({ - selectedPlugin: pluginClazz.id, - selectedApp: client.query.app, - }), - ); - store.dispatch( selectPlugin({ selectedPlugin: pluginClazz.id, @@ -137,8 +160,8 @@ export async function createMockFlipperWithPlugin( client, device: device as any, store, - sendMessage(method, params) { - client.onMessage( + sendMessage(method, params, actualClient = client) { + actualClient.onMessage( JSON.stringify({ method: 'execute', params: { @@ -152,6 +175,7 @@ export async function createMockFlipperWithPlugin( createDevice, createClient, logger, + pluginKey: getPluginKey(client.id, device, pluginClazz.id), }; } diff --git a/desktop/app/src/utils/__tests__/messageQueue.node.tsx b/desktop/app/src/utils/__tests__/messageQueue.node.tsx index ab9f21c75..585605ae9 100644 --- a/desktop/app/src/utils/__tests__/messageQueue.node.tsx +++ b/desktop/app/src/utils/__tests__/messageQueue.node.tsx @@ -57,7 +57,7 @@ class TestPlugin extends FlipperPlugin { function starTestPlugin(store: Store, client: Client) { store.dispatch( starPlugin({ - selectedPlugin: TestPlugin.id, + plugin: TestPlugin, selectedApp: client.query.app, }), ); @@ -214,7 +214,7 @@ test('queue - events are queued for plugins that are favorite when app is not se selectDeviceLogs(store); expect(store.getState().connections.selectedPlugin).not.toBe('TestPlugin'); - const client2 = createClient(device, 'TestApp2'); + const client2 = await createClient(device, 'TestApp2'); store.dispatch(selectClient(client2.id)); // Now we send a message to the second client, it should arrive, @@ -248,27 +248,39 @@ test('queue - events are queued for plugins that are favorite when app is select expect(store.getState().connections.selectedPlugin).not.toBe('TestPlugin'); const device2 = createDevice('serial2'); - const client2 = createClient(device2, client.query.app); // same app id + const client2 = await createClient(device2, client.query.app); // same app id store.dispatch(selectDevice(device2)); store.dispatch(selectClient(client2.id)); - // Now we send a message to the second client, it should arrive, + // Now we send a message to the first and second client, it should arrive, // as the plugin was enabled already on the first client as well sendMessage('inc', {delta: 2}); + sendMessage('inc', {delta: 3}, client2); + client.flushMessageBuffer(); + client2.flushMessageBuffer(); expect(store.getState().pluginStates).toMatchInlineSnapshot(`Object {}`); expect(store.getState().pluginMessageQueue).toMatchInlineSnapshot(` - Object { - "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Array [ - Object { - "api": "TestPlugin", - "method": "inc", - "params": Object { - "delta": 2, - }, - }, - ], - } - `); + Object { + "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Array [ + Object { + "api": "TestPlugin", + "method": "inc", + "params": Object { + "delta": 2, + }, + }, + ], + "TestApp#Android#MockAndroidDevice#serial2#TestPlugin": Array [ + Object { + "api": "TestPlugin", + "method": "inc", + "params": Object { + "delta": 3, + }, + }, + ], + } + `); }); test('queue - events processing will be paused', async () => { diff --git a/desktop/flipper-plugin/src/plugin/Plugin.tsx b/desktop/flipper-plugin/src/plugin/Plugin.tsx index 4bfc6f1c0..65fe2ce48 100644 --- a/desktop/flipper-plugin/src/plugin/Plugin.tsx +++ b/desktop/flipper-plugin/src/plugin/Plugin.tsx @@ -42,25 +42,40 @@ export type FlipperPluginModule = { // devicePlugin: FlipperPluginFactory }; -export class FlipperPluginInstance { +export class SandyPluginInstance { /** base client provided by Flipper */ realClient: RealFlipperClient; /** client that is bound to this instance */ client: FlipperClient; /** the original plugin definition */ - definition: FlipperPluginModule; + definition: SandyPluginDefinition; /** the plugin instance api as used inside components and such */ instanceApi: object; - constructor(realClient: RealFlipperClient, definition: FlipperPluginModule) { + constructor( + realClient: RealFlipperClient, + definition: SandyPluginDefinition, + ) { this.realClient = realClient; this.definition = definition; this.client = {}; - this.instanceApi = definition.plugin(this.client); + this.instanceApi = definition.module.plugin(this.client); + } + + activate() { + // TODO: T68683507 } deactivate() { - // TODO: + // TODO: T68683507 + } + + destroy() { + // TODO: T68683507 + } + + toJSON() { + // TODO: T68683449 } }