Fix bug with plugin re-installation after uninstallation

Summary:
Fixed a bug when plugin installation status wouldn't be saved between sessions when plugin uninstalled and then re-installed again. Before the fix, after Flipper restart, such plugin was uninstalled again because its package name was not removed from "uninstalledPlugins" state. This was because plugin id was used by mistake instead of name in few places.

To try avoiding this issue in future I've also renamed "uninstalledPlugins" to "uninstalledPluginNames" to make it more clear than package name should be used there rather than ID. As this field is persisted, I also added migration which moves data to the renamed field.

Reviewed By: passy

Differential Revision: D28314447

fbshipit-source-id: fbe3edc258b78fe7fbb0d966f93aabcdf3b66d4b

desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap

@@ -67,6 +67,6 @@ Object {
   "loadedPlugins": Map {},
   "marketplacePlugins": Array [],
   "selectedPlugins": Array [],
-  "uninstalledPlugins": Set {},
+  "uninstalledPluginNames": Set {},
 }
 `;

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

@@ -153,7 +153,9 @@ test('uninstall plugin', async () => {
     mockFlipper.getState().plugins.loadedPlugins.has('plugin1'),
   ).toBeFalsy();
   expect(
-    mockFlipper.getState().plugins.uninstalledPlugins.has('flipper-plugin1'),
+    mockFlipper
+      .getState()
+      .plugins.uninstalledPluginNames.has('flipper-plugin1'),
   ).toBeTruthy();
   expect(mockClient.sandyPluginStates.has('plugin1')).toBeFalsy();
 });
@@ -179,7 +181,7 @@ test('uninstall bundled plugin', async () => {
   expect(
     mockFlipper
       .getState()
-      .plugins.uninstalledPlugins.has('flipper-bundled-plugin'),
+      .plugins.uninstalledPluginNames.has('flipper-bundled-plugin'),
   ).toBeTruthy();
   expect(mockClient.sandyPluginStates.has('bundled-plugin')).toBeFalsy();
 });

desktop/app/src/dispatcher/pluginManager.tsx

@@ -58,7 +58,7 @@ import {
 const maxInstalledPluginVersionsToKeep = 2;
 
 function refreshInstalledPlugins(store: Store) {
-  removePlugins(store.getState().plugins.uninstalledPlugins.values())
+  removePlugins(store.getState().plugins.uninstalledPluginNames.values())
     .then(() =>
       cleanupOldInstalledPluginVersions(maxInstalledPluginVersionsToKeep),
     )

desktop/app/src/dispatcher/plugins.tsx

@@ -85,14 +85,15 @@ export default async (store: Store, logger: Logger) => {
     ),
   );
 
-  const uninstalledPlugins = store.getState().plugins.uninstalledPlugins;
+  const uninstalledPluginNames =
+    store.getState().plugins.uninstalledPluginNames;
 
   const bundledPlugins = getBundledPlugins();
 
   const loadedPlugins = filterNewestVersionOfEachPlugin(
     bundledPlugins,
     await getDynamicPlugins(),
-  ).filter((p) => !uninstalledPlugins.has(p.name));
+  ).filter((p) => !uninstalledPluginNames.has(p.name));
 
   const initialPlugins: PluginDefinition[] = loadedPlugins
     .map(reportVersion)

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

@@ -40,7 +40,7 @@ test('add clientPlugin', () => {
       disabledPlugins: [],
       selectedPlugins: [],
       marketplacePlugins: [],
-      uninstalledPlugins: new Set(),
+      uninstalledPluginNames: new Set(),
       installedPlugins: new Map(),
       initialised: false,
     },
@@ -61,7 +61,7 @@ test('add devicePlugin', () => {
       disabledPlugins: [],
       selectedPlugins: [],
       marketplacePlugins: [],
-      uninstalledPlugins: new Set(),
+      uninstalledPluginNames: new Set(),
       installedPlugins: new Map(),
       initialised: false,
     },
@@ -82,7 +82,7 @@ test('do not add plugin twice', () => {
       disabledPlugins: [],
       selectedPlugins: [],
       marketplacePlugins: [],
-      uninstalledPlugins: new Set(),
+      uninstalledPluginNames: new Set(),
       installedPlugins: new Map(),
       initialised: false,
     },
@@ -120,7 +120,7 @@ test('add gatekeeped plugin', () => {
       selectedPlugins: [],
       marketplacePlugins: [],
       installedPlugins: new Map(),
-      uninstalledPlugins: new Set(),
+      uninstalledPluginNames: new Set(),
       initialised: false,
     },
     addGatekeepedPlugins(gatekeepedPlugins),

desktop/app/src/reducers/index.tsx

@@ -33,6 +33,8 @@ import notifications, {
 import plugins, {
   State as PluginsState,
   Action as PluginsAction,
+  persistMigrations as pluginsPersistMigrations,
+  persistVersion as pluginsPersistVersion,
 } from './plugins';
 import supportForm, {
   State as SupportFormState,
@@ -169,8 +171,10 @@ export default combineReducers<State, Actions>({
     {
       key: 'plugins',
       storage,
-      whitelist: ['marketplacePlugins', 'uninstalledPlugins'],
+      whitelist: ['marketplacePlugins', 'uninstalledPluginNames'],
-      transforms: [setTransformer({whitelist: ['uninstalledPlugins']})],
+      transforms: [setTransformer({whitelist: ['uninstalledPluginNames']})],
+      version: pluginsPersistVersion,
+      migrate: createMigrate(pluginsPersistMigrations),
     },
     plugins,
   ),

desktop/app/src/reducers/plugins.tsx

@@ -27,7 +27,9 @@ export interface MarketplacePluginDetails extends DownloadablePluginDetails {
   availableVersions?: DownloadablePluginDetails[];
 }
 
-export type State = {
+export type State = StateV1;
+
+type StateV1 = {
   devicePlugins: DevicePluginMap;
   clientPlugins: ClientPluginMap;
   loadedPlugins: Map<string, ActivatablePluginDetails>;
@@ -37,11 +39,27 @@ export type State = {
   failedPlugins: Array<[ActivatablePluginDetails, string]>;
   selectedPlugins: Array<string>;
   marketplacePlugins: Array<MarketplacePluginDetails>;
-  uninstalledPlugins: Set<string>;
+  uninstalledPluginNames: Set<string>;
   installedPlugins: Map<string, InstalledPluginDetails>;
   initialised: boolean;
 };
 
+type StateV0 = Omit<StateV1, 'uninstalledPluginNames'> & {
+  uninstalledPlugins: Set<string>;
+};
+
+export const persistVersion = 1;
+export const persistMigrations = {
+  1: (state: any) => {
+    const stateV0 = state as StateV0;
+    const stateV1: StateV1 = {
+      ...stateV0,
+      uninstalledPluginNames: new Set(stateV0.uninstalledPlugins),
+    };
+    return stateV1 as any;
+  },
+};
+
 export type RegisterPluginAction = {
   type: 'REGISTER_PLUGINS';
   payload: PluginDefinition[];
@@ -107,7 +125,7 @@ const INITIAL_STATE: State = {
   failedPlugins: [],
   selectedPlugins: [],
   marketplacePlugins: [],
-  uninstalledPlugins: new Set(),
+  uninstalledPluginNames: new Set(),
   installedPlugins: new Map(),
   initialised: false,
 };
@@ -169,7 +187,7 @@ export default function reducer(
     return produce(state, (draft) => {
       draft.installedPlugins.clear();
       action.payload.forEach((p) => {
-        if (!draft.uninstalledPlugins.has(p.id)) {
+        if (!draft.uninstalledPluginNames.has(p.name)) {
           draft.installedPlugins.set(p.id, p);
         }
       });
@@ -188,7 +206,7 @@ export default function reducer(
       draft.clientPlugins.delete(plugin.id);
       draft.devicePlugins.delete(plugin.id);
       draft.loadedPlugins.delete(plugin.id);
-      draft.uninstalledPlugins.add(plugin.name);
+      draft.uninstalledPluginNames.add(plugin.name);
     });
   } else if (action.type === 'PLUGIN_LOADED') {
     const plugin = action.payload;
@@ -198,7 +216,7 @@ export default function reducer(
       } else {
         draft.clientPlugins.set(plugin.id, plugin);
       }
-      draft.uninstalledPlugins.delete(plugin.id);
+      draft.uninstalledPluginNames.delete(plugin.details.name);
       draft.loadedPlugins.set(plugin.id, plugin.details);
     });
   } else if (action.type === 'PLUGINS_INITIALISED') {

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

@@ -767,7 +767,7 @@ test('test determinePluginsToProcess for mutilple clients having plugins present
     selectedPlugins: ['TestPlugin'],
     marketplacePlugins: [],
     installedPlugins: new Map(),
-    uninstalledPlugins: new Set(),
+    uninstalledPluginNames: new Set(),
     initialised: true,
   };
   const op = determinePluginsToProcess(
@@ -842,7 +842,7 @@ test('test determinePluginsToProcess for no selected plugin present in any clien
     selectedPlugins: ['RandomPlugin'],
     marketplacePlugins: [],
     installedPlugins: new Map(),
-    uninstalledPlugins: new Set(),
+    uninstalledPluginNames: new Set(),
     initialised: true,
   };
   const op = determinePluginsToProcess([client1, client2], device1, plugins);
@@ -894,7 +894,7 @@ test('test determinePluginsToProcess for multiple clients on same device', async
     selectedPlugins: ['TestPlugin'],
     marketplacePlugins: [],
     installedPlugins: new Map(),
-    uninstalledPlugins: new Set(),
+    uninstalledPluginNames: new Set(),
     initialised: true,
   };
   const op = determinePluginsToProcess([client1, client2], device1, plugins);
@@ -984,7 +984,7 @@ test('test determinePluginsToProcess for multiple clients on different device',
     selectedPlugins: ['TestPlugin'],
     marketplacePlugins: [],
     installedPlugins: new Map(),
-    uninstalledPlugins: new Set(),
+    uninstalledPluginNames: new Set(),
     initialised: true,
   };
   const op = determinePluginsToProcess(
@@ -1070,7 +1070,7 @@ test('test determinePluginsToProcess to ignore archived clients', async () => {
     selectedPlugins: ['TestPlugin'],
     marketplacePlugins: [],
     installedPlugins: new Map(),
-    uninstalledPlugins: new Set(),
+    uninstalledPluginNames: new Set(),
     initialised: true,
   };
   const op = determinePluginsToProcess(