Command processing (5/n): Star plugin

Summary:
*Stack summary*: this stack refactors plugin management actions to perform them in a dispatcher rather than in the root reducer (store.tsx) as all of these actions has side effects. To do that, we store requested plugin management actions (install/update/uninstall, star/unstar) in a queue which is then handled by pluginManager dispatcher. This dispatcher then dispatches all required state updates.

*Diff summary*: refactored "star plugin" operation to perform it in pluginManager dispatcher.

Reviewed By: mweststrate

Differential Revision: D26305576

fbshipit-source-id: 90516af4e9ba8504720ddfa587f691f53e71b702

desktop/app/src/PluginContainer.tsx

@@ -33,8 +33,8 @@ import {
   StaticView,
   setStaticView,
   pluginIsStarred,
-  starPlugin,
 } from './reducers/connections';
+import {starPlugin} from './reducers/pluginManager';
 import React, {PureComponent} from 'react';
 import {connect, ReactReduxContext} from 'react-redux';
 import {setPluginState} from './reducers/pluginStates';

desktop/app/src/__tests__/PluginContainer.node.tsx

@@ -21,8 +21,9 @@ import {
   DeviceLogEntry,
   useValue,
 } from 'flipper-plugin';
-import {selectPlugin, starPlugin} from '../reducers/connections';
+import {selectPlugin} from '../reducers/connections';
 import {updateSettings} from '../reducers/settings';
+import {starPlugin} from '../reducers/pluginManager';
 
 interface PersistedState {
   count: 1;

desktop/app/src/dispatcher/__tests__/pluginManager.node.tsx

@@ -9,13 +9,18 @@
 
 jest.mock('../plugins');
 jest.mock('../../utils/electronModuleCache');
-import {loadPlugin, uninstallPlugin} from '../../reducers/pluginManager';
+import {
+  loadPlugin,
+  starPlugin,
+  uninstallPlugin,
+} from '../../reducers/pluginManager';
 import {requirePlugin} from '../plugins';
 import {mocked} from 'ts-jest/utils';
 import {TestUtils} from 'flipper-plugin';
 import * as TestPlugin from '../../test-utils/TestPlugin';
 import {_SandyPluginDefinition as SandyPluginDefinition} from 'flipper-plugin';
 import MockFlipper from '../../test-utils/MockFlipper';
+import Client from '../../Client';
 
 const pluginDetails1 = TestUtils.createMockPluginDetails({
   id: 'plugin1',
@@ -43,6 +48,7 @@ const pluginDefinition2 = new SandyPluginDefinition(pluginDetails2, TestPlugin);
 const mockedRequirePlugin = mocked(requirePlugin);
 
 let mockFlipper: MockFlipper;
+let mockClient: Client;
 
 beforeEach(async () => {
   mockedRequirePlugin.mockImplementation(
@@ -56,9 +62,10 @@ beforeEach(async () => {
         : undefined)!,
   );
   mockFlipper = new MockFlipper();
-  await mockFlipper.initWithDeviceAndClient({
+  const initResult = await mockFlipper.initWithDeviceAndClient({
     clientOptions: {supportedPlugins: ['plugin1', 'plugin2']},
   });
+  mockClient = initResult.client;
 });
 
 afterEach(async () => {
@@ -76,7 +83,7 @@ test('load plugin when no other version loaded', async () => {
   expect(mockFlipper.getState().plugins.loadedPlugins.get('plugin1')).toBe(
     pluginDetails1,
   );
-  expect(mockFlipper.clients[0].sandyPluginStates.has('plugin1')).toBeFalsy();
+  expect(mockClient.sandyPluginStates.has('plugin1')).toBeFalsy();
 });
 
 test('load plugin when other version loaded', async () => {
@@ -96,7 +103,7 @@ test('load plugin when other version loaded', async () => {
   expect(mockFlipper.getState().plugins.loadedPlugins.get('plugin1')).toBe(
     pluginDetails1V2,
   );
-  expect(mockFlipper.clients[0].sandyPluginStates.has('plugin1')).toBeFalsy();
+  expect(mockClient.sandyPluginStates.has('plugin1')).toBeFalsy();
 });
 
 test('load and enable Sandy plugin', async () => {
@@ -109,7 +116,7 @@ test('load and enable Sandy plugin', async () => {
   expect(mockFlipper.getState().plugins.loadedPlugins.get('plugin1')).toBe(
     pluginDetails1,
   );
-  expect(mockFlipper.clients[0].sandyPluginStates.has('plugin1')).toBeTruthy();
+  expect(mockClient.sandyPluginStates.has('plugin1')).toBeTruthy();
 });
 
 test('uninstall plugin', async () => {
@@ -126,7 +133,7 @@ test('uninstall plugin', async () => {
   expect(
     mockFlipper.getState().plugins.uninstalledPlugins.has('flipper-plugin1'),
   ).toBeTruthy();
-  expect(mockFlipper.clients[0].sandyPluginStates.has('plugin1')).toBeFalsy();
+  expect(mockClient.sandyPluginStates.has('plugin1')).toBeFalsy();
 });
 
 test('uninstall bundled plugin', async () => {
@@ -152,7 +159,43 @@ test('uninstall bundled plugin', async () => {
       .getState()
       .plugins.uninstalledPlugins.has('flipper-bundled-plugin'),
   ).toBeTruthy();
-  expect(
+  expect(mockClient.sandyPluginStates.has('bundled-plugin')).toBeFalsy();
-    mockFlipper.clients[0].sandyPluginStates.has('bundled-plugin'),
+});
-  ).toBeFalsy();
+
+test('star plugin', async () => {
+  mockFlipper.dispatch(
+    loadPlugin({plugin: pluginDetails1, enable: false, notifyIfFailed: false}),
+  );
+  mockFlipper.dispatch(
+    starPlugin({
+      plugin: pluginDefinition1,
+      selectedApp: mockClient.query.app,
+    }),
+  );
+  expect(
+    mockFlipper.getState().connections.userStarredPlugins[mockClient.query.app],
+  ).toContain('plugin1');
+  expect(mockClient.sandyPluginStates.has('plugin1')).toBeTruthy();
+});
+
+test('unstar plugin', async () => {
+  mockFlipper.dispatch(
+    loadPlugin({plugin: pluginDetails1, enable: false, notifyIfFailed: false}),
+  );
+  mockFlipper.dispatch(
+    starPlugin({
+      plugin: pluginDefinition1,
+      selectedApp: mockClient.query.app,
+    }),
+  );
+  mockFlipper.dispatch(
+    starPlugin({
+      plugin: pluginDefinition1,
+      selectedApp: mockClient.query.app,
+    }),
+  );
+  expect(
+    mockFlipper.getState().connections.userStarredPlugins[mockClient.query.app],
+  ).not.toContain('plugin1');
+  expect(mockClient.sandyPluginStates.has('plugin1')).toBeFalsy();
 });

desktop/app/src/dispatcher/pluginManager.tsx

@@ -16,6 +16,7 @@ import {
   UninstallPluginActionPayload,
   UpdatePluginActionPayload,
   pluginCommandsProcessed,
+  StarPluginActionPayload,
 } from '../reducers/pluginManager';
 import {
   getInstalledPlugins,
@@ -41,8 +42,13 @@ import {
 } from '../reducers/plugins';
 import {_SandyPluginDefinition} from 'flipper-plugin';
 import type BaseDevice from '../devices/BaseDevice';
-import {pluginStarred} from '../reducers/connections';
+import {pluginStarred, pluginUnstarred} from '../reducers/connections';
-import {defaultEnabledBackgroundPlugins} from '../utils/pluginUtils';
+import {deconstructClientId} from '../utils/clientUtils';
+import {clearMessageQueue} from '../reducers/pluginMessageQueue';
+import {
+  getPluginKey,
+  defaultEnabledBackgroundPlugins,
+} from '../utils/pluginUtils';
 
 const maxInstalledPluginVersionsToKeep = 2;
 
@@ -101,6 +107,9 @@ export function processPluginCommandsQueue(
       case 'UPDATE_PLUGIN':
         updatePlugin(store, command.payload);
         break;
+      case 'STAR_PLUGIN':
+        starPlugin(store, command.payload);
+        break;
       default:
         console.error('Unexpected plugin command', command);
         break;
@@ -159,6 +168,39 @@ function updatePlugin(store: Store, payload: UpdatePluginActionPayload) {
   }
 }
 
+function getSelectedAppId(store: Store) {
+  const {connections} = store.getState();
+  const selectedApp = connections.selectedApp
+    ? deconstructClientId(connections.selectedApp).app
+    : undefined;
+  return selectedApp;
+}
+
+function starPlugin(store: Store, payload: StarPluginActionPayload) {
+  const {plugin, selectedApp} = payload;
+  const {connections} = store.getState();
+  const clients = connections.clients.filter(
+    (client) => client.query.app === selectedApp,
+  );
+  if (connections.userStarredPlugins[selectedApp]?.includes(plugin.id)) {
+    clients.forEach((client) => {
+      stopPlugin(client, plugin.id);
+      const pluginKey = getPluginKey(
+        client.id,
+        {serial: client.query.device_id},
+        plugin.id,
+      );
+      store.dispatch(clearMessageQueue(pluginKey));
+    });
+    store.dispatch(pluginUnstarred(plugin, selectedApp));
+  } else {
+    clients.forEach((client) => {
+      startPlugin(client, plugin);
+    });
+    store.dispatch(pluginStarred(plugin, selectedApp));
+  }
+}
+
 function updateClientPlugin(
   store: Store,
   plugin: typeof FlipperPlugin,
@@ -166,7 +208,10 @@ function updateClientPlugin(
 ) {
   const clients = store.getState().connections.clients;
   if (enable) {
-    store.dispatch(pluginStarred(plugin));
+    const selectedApp = getSelectedAppId(store);
+    if (selectedApp) {
+      store.dispatch(pluginStarred(plugin, selectedApp));
+    }
   }
   const clientsWithEnabledPlugin = clients.filter((c) => {
     return (

desktop/app/src/reducers/__tests__/sandyplugins.node.tsx

@@ -9,7 +9,7 @@
 
 import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin';
 import {Store, Client} from '../../';
-import {selectPlugin, starPlugin} from '../../reducers/connections';
+import {selectPlugin} from '../../reducers/connections';
 import {registerPlugins} from '../../reducers/plugins';
 import {
   _SandyPluginDefinition,
@@ -17,6 +17,7 @@ import {
   PluginClient,
   TestUtils,
 } from 'flipper-plugin';
+import {starPlugin} from '../pluginManager';
 
 const pluginDetails = TestUtils.createMockPluginDetails();
 

desktop/app/src/reducers/connections.tsx

@@ -108,24 +108,18 @@ export type Action =
       payload: StaticView;
       deepLinkPayload: unknown;
     }
-  | {
-      // Implemented by rootReducer in `store.tsx`
-      type: 'STAR_PLUGIN';
-      payload: {
-        selectedApp: string;
-        plugin: PluginDefinition;
-      };
-    }
   | {
       type: 'PLUGIN_STARRED';
       payload: {
         plugin: PluginDefinition;
+        selectedApp: string;
       };
     }
   | {
       type: 'PLUGIN_UNSTARRED';
       payload: {
         plugin: PluginDefinition;
+        selectedApp: string;
       };
     }
   | {
@@ -372,14 +366,8 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
       return state;
     }
     case 'PLUGIN_STARRED': {
-      const {plugin} = action.payload;
+      const {plugin, selectedApp} = action.payload;
       const selectedPlugin = plugin.id;
-      const selectedApp = state.selectedApp
-        ? deconstructClientId(state.selectedApp).app
-        : undefined;
-      if (!selectedApp) {
-        return state;
-      }
       return produce(state, (draft) => {
         if (!draft.userStarredPlugins[selectedApp]) {
           draft.userStarredPlugins[selectedApp] = [];
@@ -392,12 +380,8 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
       });
     }
     case 'PLUGIN_UNSTARRED': {
-      const {plugin} = action.payload;
+      const {plugin, selectedApp} = action.payload;
       const selectedPlugin = plugin.id;
-      const selectedApp = state.selectedApp;
-      if (!selectedApp) {
-        return state;
-      }
       return produce(state, (draft) => {
         if (!draft.userStarredPlugins[selectedApp]) {
           draft.userStarredPlugins[selectedApp] = [];
@@ -449,30 +433,30 @@ export const selectPlugin = (payload: {
   payload: {...payload, time: payload.time ?? Date.now()},
 });
 
-export const starPlugin = (payload: {
-  plugin: PluginDefinition;
-  selectedApp: string;
-}): Action => ({
-  type: 'STAR_PLUGIN',
-  payload,
-});
-
 export const selectClient = (clientId: string | null): Action => ({
   type: 'SELECT_CLIENT',
   payload: clientId,
 });
 
-export const pluginStarred = (plugin: PluginDefinition): Action => ({
+export const pluginStarred = (
+  plugin: PluginDefinition,
+  appId: string,
+): Action => ({
   type: 'PLUGIN_STARRED',
   payload: {
     plugin,
+    selectedApp: appId,
   },
 });
 
-export const pluginUnstarred = (plugin: PluginDefinition): Action => ({
+export const pluginUnstarred = (
+  plugin: PluginDefinition,
+  appId: string,
+): Action => ({
   type: 'PLUGIN_UNSTARRED',
   payload: {
     plugin,
+    selectedApp: appId,
   },
 });
 

desktop/app/src/reducers/pluginManager.tsx

@@ -19,7 +19,8 @@ export type State = {
 export type PluginCommand =
   | LoadPluginAction
   | UninstallPluginAction
-  | UpdatePluginAction;
+  | UpdatePluginAction
+  | StarPluginAction;
 
 export type LoadPluginActionPayload = {
   plugin: ActivatablePluginDetails;
@@ -51,6 +52,16 @@ export type UpdatePluginAction = {
   payload: UpdatePluginActionPayload;
 };
 
+export type StarPluginActionPayload = {
+  plugin: PluginDefinition;
+  selectedApp: string;
+};
+
+export type StarPluginAction = {
+  type: 'STAR_PLUGIN';
+  payload: StarPluginActionPayload;
+};
+
 export type Action =
   | {
       type: 'PLUGIN_COMMANDS_PROCESSED';
@@ -70,6 +81,7 @@ export default function reducer(
     case 'LOAD_PLUGIN':
     case 'UNINSTALL_PLUGIN':
     case 'UPDATE_PLUGIN':
+    case 'STAR_PLUGIN':
       return produce(state, (draft) => {
         draft.pluginCommandsQueue.push(action);
       });
@@ -105,3 +117,8 @@ export const registerPluginUpdate = (
   type: 'UPDATE_PLUGIN',
   payload,
 });
+
+export const starPlugin = (payload: StarPluginActionPayload): Action => ({
+  type: 'STAR_PLUGIN',
+  payload,
+});

desktop/app/src/reducers/pluginMessageQueue.tsx

@@ -34,7 +34,7 @@ export type Action =
       type: 'CLEAR_MESSAGE_QUEUE';
       payload: {
         pluginKey: string; // client + plugin
-        amount: number;
+        amount?: number;
       };
     }
   | {
@@ -75,7 +75,11 @@ export default function reducer(
       return produce(state, (draft) => {
         const messages = draft[pluginKey];
         if (messages) {
-          messages.splice(0, amount);
+          if (amount === undefined) {
+            delete draft[pluginKey];
+          } else {
+            messages.splice(0, amount);
+          }
         }
       });
     }
@@ -130,7 +134,7 @@ export const queueMessages = (
 
 export const clearMessageQueue = (
   pluginKey: string,
-  amount: number,
+  amount?: number,
 ): Action => ({
   type: 'CLEAR_MESSAGE_QUEUE',
   payload: {pluginKey, amount},

desktop/app/src/reducers/supportForm.tsx

@@ -10,7 +10,7 @@
 import {Actions, Store} from './';
 import {setStaticView} from './connections';
 import {deconstructClientId} from '../utils/clientUtils';
-import {starPlugin as setStarPlugin} from './connections';
+import {starPlugin as setStarPlugin} from './pluginManager';
 import {showStatusUpdatesForDuration} from '../utils/promiseTimeout';
 import {selectedPlugins as setSelectedPlugins} from './plugins';
 import {addStatusMessage, removeStatusMessage} from './application';

desktop/app/src/sandy-chrome/appinspect/PluginList.tsx

@@ -25,7 +25,7 @@ import {
   getPluginTitle,
   getPluginTooltip,
 } from '../../utils/pluginUtils';
-import {selectPlugin, starPlugin} from '../../reducers/connections';
+import {selectPlugin} from '../../reducers/connections';
 import Client from '../../Client';
 import BaseDevice from '../../devices/BaseDevice';
 import {DownloadablePluginDetails} from 'flipper-plugin-lib';
@@ -36,7 +36,11 @@ import {
   PluginDownloadStatus,
   startPluginDownload,
 } from '../../reducers/pluginDownloads';
-import {loadPlugin, uninstallPlugin} from '../../reducers/pluginManager';
+import {
+  loadPlugin,
+  starPlugin,
+  uninstallPlugin,
+} from '../../reducers/pluginManager';
 import {BundledPluginDetails} from 'plugin-lib';
 import {reportUsage} from '../../utils/metrics';
 

desktop/app/src/sandy-chrome/appinspect/__tests__/PluginList.spec.tsx

@@ -17,13 +17,14 @@ import MetroDevice from '../../../devices/MetroDevice';
 import BaseDevice from '../../../devices/BaseDevice';
 import {_SandyPluginDefinition} from 'flipper-plugin';
 import {createMockPluginDetails} from 'flipper-plugin/src/test-utils/test-utils';
-import {selectPlugin, starPlugin} from '../../../reducers/connections';
+import {selectPlugin} from '../../../reducers/connections';
 import {registerMetroDevice} from '../../../dispatcher/metroDevice';
 import {
   addGatekeepedPlugins,
   registerMarketplacePlugins,
   registerPlugins,
 } from '../../../reducers/plugins';
+import {starPlugin} from '../../../reducers/pluginManager';
 
 // eslint-disable-next-line
 import * as LogsPluginModule from '../../../../../plugins/logs/index';

desktop/app/src/store.tsx

@@ -11,13 +11,6 @@ import {createStore} from 'redux';
 import reducers, {Actions, State as StoreState, Store} from './reducers/index';
 import {stateSanitizer} from './utils/reduxDevToolsConfig';
 import isProduction from './utils/isProduction';
-import produce from 'immer';
-import {
-  defaultEnabledBackgroundPlugins,
-  getPluginKey,
-} from './utils/pluginUtils';
-import Client from './Client';
-import {PluginDefinition} from './plugin';
 import {_SandyPluginDefinition} from 'flipper-plugin';
 export const store: Store = createStore<StoreState, Actions, any, any>(
   rootReducer,
@@ -34,41 +27,6 @@ export function rootReducer(
   state: StoreState | undefined,
   action: Actions,
 ): StoreState {
-  if (action.type === 'STAR_PLUGIN' && state) {
-    const {plugin, selectedApp} = action.payload;
-    const selectedPlugin = plugin.id;
-    const clients = state.connections.clients.filter(
-      (client) => client.query.app === selectedApp,
-    );
-    return produce(state, (draft) => {
-      if (!draft.connections.userStarredPlugins[selectedApp]) {
-        draft.connections.userStarredPlugins[selectedApp] = [];
-      }
-      const plugins = draft.connections.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) => {
-          startPlugin(client, plugin);
-        });
-      } else {
-        plugins.splice(idx, 1);
-        // enabling a plugin on one device disables it on all...
-        clients.forEach((client) => {
-          stopPlugin(client, plugin.id);
-          const pluginKey = getPluginKey(
-            client.id,
-            {serial: client.query.device_id},
-            plugin.id,
-          );
-          delete draft.pluginMessageQueue[pluginKey];
-        });
-      }
-    });
-  }
-
-  // otherwise
   return reducers(state, action);
 }
 
@@ -77,36 +35,3 @@ if (!isProduction()) {
   // @ts-ignore
   window.flipperStore = store;
 }
-
-function stopPlugin(
-  client: Client,
-  pluginId: string,
-  forceInitBackgroundPlugin: boolean = false,
-): boolean {
-  if (
-    (forceInitBackgroundPlugin ||
-      !defaultEnabledBackgroundPlugins.includes(pluginId)) &&
-    client?.isBackgroundPlugin(pluginId)
-  ) {
-    client.deinitPlugin(pluginId);
-  }
-  // stop sandy plugins
-  client.stopPluginIfNeeded(pluginId);
-  return true;
-}
-
-function startPlugin(
-  client: Client,
-  plugin: PluginDefinition,
-  forceInitBackgroundPlugin: boolean = false,
-) {
-  client.startPluginIfNeeded(plugin, true);
-  // background plugin? connect it needed
-  if (
-    (forceInitBackgroundPlugin ||
-      !defaultEnabledBackgroundPlugins.includes(plugin.id)) &&
-    client?.isBackgroundPlugin(plugin.id)
-  ) {
-    client.initPlugin(plugin.id);
-  }
-}

desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx

@@ -18,7 +18,6 @@ import {queries} from '@testing-library/dom';
 
 import {
   selectPlugin,
-  starPlugin,
   selectDevice,
   selectClient,
 } from '../reducers/connections';
@@ -32,6 +31,7 @@ import {PluginDefinition} from '../plugin';
 import PluginContainer from '../PluginContainer';
 import {getPluginKey, isDevicePluginDefinition} from '../utils/pluginUtils';
 import MockFlipper from './MockFlipper';
+import {starPlugin} from '../reducers/pluginManager';
 
 export type MockFlipperResult = {
   client: Client;

desktop/app/src/utils/__tests__/messageQueue.node.tsx

@@ -12,7 +12,6 @@ import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWit
 import {Store, Client, sleep} from '../../';
 import {
   selectPlugin,
-  starPlugin,
   selectClient,
   selectDevice,
 } from '../../reducers/connections';
@@ -24,6 +23,7 @@ import pluginMessageQueue, {
   queueMessages,
 } from '../../reducers/pluginMessageQueue';
 import {registerPlugins} from '../../reducers/plugins';
+import {starPlugin} from '../../reducers/pluginManager';
 
 interface PersistedState {
   count: 1;

desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx

@@ -12,7 +12,6 @@ import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWit
 import {Store, Client, sleep} from '../../';
 import {
   selectPlugin,
-  starPlugin,
   selectClient,
   selectDevice,
 } from '../../reducers/connections';
@@ -26,6 +25,7 @@ import {
   PluginClient,
   _SandyPluginInstance,
 } from 'flipper-plugin';
+import {starPlugin} from '../../reducers/pluginManager';
 
 type Events = {
   inc: {