Replace all manual pluginKey parsing with a utility

Summary:
Ok this diff got a bit bigger than expected, but I think it makes it easier to understand what "plugin keys" are, and makes them less prone to error.

Previously pluginKeys were composed of: clientId#pluginName, where clientId was itself: app#os#device#device_id

But also, there were some plugin keys where the clientId was a device_id.

Now you deconstruct a plugin key, and will get a tagged object with type: 'device' or 'client', and the properties that they each have.

There is now no custom parsing of these afaik, let's keep it that way.

Since it took me a while to figure out what all these IDs are, I've documented it a bit in clientUtils.

Reviewed By: passy

Differential Revision: D18811848

fbshipit-source-id: eed2e2b5eedafb9e27900dbcf79a389fcaffae95

src/dispatcher/notifications.tsx

@@ -21,6 +21,7 @@ import {
 } from '../reducers/notifications';
 import {textContent} from '../utils/index';
 import GK from '../fb-stubs/GK';
+import {deconstructPluginKey} from '../utils/clientUtils';
 
 type NotificationEvents = 'show' | 'click' | 'close' | 'reply' | 'action';
 const NOTIFICATION_THROTTLE = 5 * 1000; // in milliseconds
@@ -106,18 +107,18 @@ export default (store: Store, logger: Logger) => {
     Object.keys(pluginStates).forEach(key => {
       if (knownPluginStates.get(key) !== pluginStates[key]) {
         knownPluginStates.set(key, pluginStates[key]);
-        const split = key.split('#');
+        const plugin = deconstructPluginKey(key);
-        const pluginId = split.pop();
+        const pluginName = plugin.pluginName;
-        const client = split.join('#');
+        const client = plugin.client;
 
-        if (!pluginId) {
+        if (!pluginName) {
           return;
         }
 
         const persistingPlugin:
           | undefined
           | typeof FlipperPlugin
-          | typeof FlipperDevicePlugin = pluginMap.get(pluginId);
+          | typeof FlipperDevicePlugin = pluginMap.get(pluginName);
         if (persistingPlugin && persistingPlugin.getActiveNotifications) {
           store.dispatch(
             setActiveNotifications({
@@ -125,7 +126,7 @@ export default (store: Store, logger: Logger) => {
                 pluginStates[key],
               ),
               client,
-              pluginId,
+              pluginId: pluginName,
             }),
           );
         }

src/reducers/pluginStates.tsx

@@ -8,6 +8,7 @@
  */
 
 import {Actions} from '.';
+import {deconstructPluginKey} from '../utils/clientUtils';
 
 export type State = {
   [pluginKey: string]: Object;
@@ -53,8 +54,9 @@ export default function reducer(
     return Object.keys(state).reduce((newState: State, pluginKey) => {
       // Only add the pluginState, if its from a plugin other than the one that
       // was removed. pluginKeys are in the form of ${clientID}#${pluginID}.
-      const clientId = pluginKey.slice(0, pluginKey.lastIndexOf('#'));
+      const plugin = deconstructPluginKey(pluginKey);
-      const pluginId = pluginKey.split('#').pop();
+      const clientId = plugin.client;
+      const pluginId = plugin.pluginName;
       if (
         clientId !== payload.clientId ||
         (pluginId && payload.devicePlugins.has(pluginId))

src/utils/clientUtils.tsx

@@ -10,6 +10,10 @@
 import Client from '../Client';
 import BaseDevice from '../devices/BaseDevice';
 
+/* A Client uniuely identifies an app running on some device.
+
+  Always use this utility to construct and parse clientId strings.
+ */
 export type ClientIdConstituents = {
   app: string;
   os: string;
@@ -17,6 +21,25 @@ export type ClientIdConstituents = {
   device_id: string;
 };
 
+/* A plugin key is a string uniquely identifying an instance of a plugin.
+   This can be a device plugin for a particular device, or a client plugin for a particular client (app).
+   In the device plugin case, the "client" is the device it's connected to.
+   In the client plugin case (normal plugins), the "client" is the app it's connected to.
+
+   Always use this utility to construct and parse pluginKey strings.
+ */
+type PluginKeyConstituents =
+  | {
+      type: 'device';
+      pluginName: string;
+      client: string;
+    }
+  | ({
+      type: 'client';
+      pluginName: string;
+      client: string;
+    } & ClientIdConstituents);
+
 export function currentActiveApps(
   clients: Array<Client>,
   selectedDevice: null | BaseDevice,
@@ -64,3 +87,31 @@ export function deconstructClientId(clientId: string): ClientIdConstituents {
     device_id,
   };
 }
+
+export function deconstructPluginKey(pluginKey: string): PluginKeyConstituents {
+  const parts = pluginKey.split('#');
+  if (parts.length === 2) {
+    // Device plugin
+    return {
+      type: 'device',
+      client: parts[0],
+      pluginName: parts[1],
+    };
+  } else {
+    // Client plugin
+    const lastHashIndex = pluginKey.lastIndexOf('#');
+    const clientId = pluginKey.slice(0, lastHashIndex);
+    const pluginName = pluginKey.slice(lastHashIndex + 1);
+    if (!pluginName) {
+      console.error(
+        `Attempted to deconstruct invalid pluginKey: "${pluginKey}"`,
+      );
+    }
+    return {
+      type: 'client',
+      ...deconstructClientId(clientId),
+      client: clientId,
+      pluginName: pluginName,
+    };
+  }
+}

src/utils/exportData.tsx

@@ -41,7 +41,7 @@ import {
   SupportFormRequestDetailsState,
 } from '../reducers/supportForm';
 import {setSelectPluginsToExportActiveSheet} from '../reducers/application';
-import {deconstructClientId} from '../utils/clientUtils';
+import {deconstructClientId, deconstructPluginKey} from '../utils/clientUtils';
 
 export const IMPORT_FLIPPER_TRACE_EVENT = 'import-flipper-trace';
 export const EXPORT_FLIPPER_TRACE_EVENT = 'export-flipper-trace';
@@ -138,8 +138,9 @@ export function processPluginStates(
   statusUpdate &&
     statusUpdate('Filtering the plugin states for the filtered Clients...');
   for (const key in allPluginStates) {
-    const keyArray = key.split('#');
+    const plugin = deconstructPluginKey(key);
-    const pluginName = keyArray.pop();
+
+    const pluginName = plugin.pluginName;
     if (
       pluginName &&
       selectedPlugins.length > 0 &&
@@ -147,18 +148,22 @@ export function processPluginStates(
     ) {
       continue;
     }
-    const filteredClients = clients.filter(client => {
+    if (plugin.type === 'client') {
-      // Remove the last entry related to plugin
+      if (!clients.some(c => c.id.includes(plugin.client))) {
-      return client.id.includes(keyArray.join('#'));
+        continue;
-    });
-    if (
-      filteredClients.length > 0 ||
-      (pluginName && devicePlugins.has(pluginName) && serial === keyArray[0])
-    ) {
-      // There need not be any client for device Plugins
-      pluginStates = {...pluginStates, [key]: allPluginStates[key]};
       }
     }
+    if (plugin.type === 'device') {
+      if (
+        !pluginName ||
+        !devicePlugins.has(pluginName) ||
+        serial !== plugin.client
+      ) {
+        continue;
+      }
+    }
+    pluginStates = {...pluginStates, [key]: allPluginStates[key]};
+  }
   return pluginStates;
 }
 
@@ -202,8 +207,7 @@ const serializePluginStates = async (
   });
   const pluginExportState: PluginStatesExportState = {};
   for (const key in pluginStates) {
-    const keyArray = key.split('#');
+    const pluginName = deconstructPluginKey(key).pluginName;
-    const pluginName = keyArray.pop();
     statusUpdate && statusUpdate(`Serialising ${pluginName}...`);
     const serializationMarker = `${EXPORT_FLIPPER_TRACE_EVENT}:serialization-per-plugin`;
     performance.mark(serializationMarker);
@@ -237,8 +241,7 @@ const deserializePluginStates = (
   });
   const pluginsState: PluginStatesState = {};
   for (const key in pluginStatesExportState) {
-    const keyArray = key.split('#');
+    const pluginName = deconstructPluginKey(key).pluginName;
-    const pluginName = keyArray.pop();
     if (!pluginName || !pluginsMap.get(pluginName)) {
       continue;
     }
@@ -655,12 +658,10 @@ export function importDataToStore(source: string, data: string, store: Store) {
   clients.forEach((client: {id: string; query: ClientQuery}) => {
     const clientPlugins: Array<string> = keys
       .filter(key => {
-        const arr = key.split('#');
+        const plugin = deconstructPluginKey(key);
-        arr.pop();
+        return plugin.type === 'client' && client.id === plugin.client;
-        const clientPlugin = arr.join('#');
-        return client.id === clientPlugin;
       })
-      .map(client => client.split('#').pop() || '');
+      .map(pluginKey => deconstructPluginKey(pluginKey).pluginName);
     store.dispatch({
       type: 'NEW_CLIENT',
       payload: new Client(

src/utils/exportMetrics.tsx

@@ -14,6 +14,7 @@ import {Store} from '../reducers';
 import fs from 'fs';
 import {ExportType, fetchMetadata, pluginsClassMap} from './exportData';
 import {deserializeObject} from './serialization';
+import {deconstructPluginKey} from './clientUtils';
 
 export type MetricType = {[metricName: string]: number};
 type MetricPluginType = {[pluginID: string]: MetricType};
@@ -27,30 +28,31 @@ async function exportMetrics(
   const metrics: ExportMetricType = {};
   for (const key in pluginStates) {
     const pluginStateData = pluginStates[key];
-    const arr = key.split('#');
+
-    const pluginName = arr.pop();
+    const plugin = deconstructPluginKey(key);
+    const pluginName = plugin.pluginName;
     if (
       pluginName === undefined ||
       (selectedPlugins.length > 0 && !selectedPlugins.includes(pluginName))
     ) {
       continue;
     }
-    const clientID = arr.join('#');
+    const client = plugin.client;
-    const plugin = pluginsMap.get(pluginName);
+    const pluginClass = pluginsMap.get(pluginName);
     const metricsReducer:
       | (<U>(persistedState: U) => Promise<MetricType>)
       | null
-      | undefined = plugin && plugin.metricsReducer;
+      | undefined = pluginClass && pluginClass.metricsReducer;
     if (pluginsMap.has(pluginName) && metricsReducer) {
       const metricsObject = await metricsReducer(pluginStateData);
       const pluginObject: MetricPluginType = {};
       pluginObject[pluginName] = metricsObject;
-      if (!metrics[clientID]) {
+      if (!metrics[client]) {
-        metrics[clientID] = pluginObject;
+        metrics[client] = pluginObject;
         continue;
       }
-      const mergedMetrics = {...metrics[clientID], ...pluginObject};
+      const mergedMetrics = {...metrics[client], ...pluginObject};
-      metrics[clientID] = mergedMetrics;
+      metrics[client] = mergedMetrics;
     }
   }
   return Promise.resolve(await serialize(metrics));

src/utils/pluginUtils.tsx

@@ -13,6 +13,7 @@ import {State as PluginStatesState} from '../reducers/pluginStates';
 import {pluginsClassMap} from './exportData';
 import {State as PluginsState} from '../reducers/plugins';
 import {PluginDefinition} from '../dispatcher/plugins';
+import {deconstructPluginKey} from './clientUtils';
 
 export function getPluginKey(
   selectedApp: string | null,
@@ -53,13 +54,15 @@ export function getActivePersistentPlugins(
     string,
     typeof FlipperDevicePlugin | typeof FlipperPlugin
   > = pluginsClassMap(plugins);
-  return getPersistentPlugins(plugins).filter(plugin => {
+  return getPersistentPlugins(plugins).filter(pluginName => {
-    const pluginClass = pluginsMap.get(plugin);
+    const pluginClass = pluginsMap.get(pluginName);
-    const keys = Object.keys(pluginsState).map(key => key.split('#').pop());
+    const pluginNames = Object.keys(pluginsState).map(
+      pluginKey => deconstructPluginKey(pluginKey).pluginName,
+    );
     return (
       (pluginClass && pluginClass.exportPersistedState != undefined) ||
-      plugin == 'DeviceLogs' ||
+      pluginName == 'DeviceLogs' ||
-      keys.includes(plugin)
+      pluginNames.includes(pluginName)
     );
   });
 }