Refactor CrashReporter plugin

Summary: This diff refactors CrashReporter Plugin. The business logic of crash reporter plugin was placed in the `iOSDevice.js` as it gets the hook to the initialisation of iOSDevice. This diff moves the business logic to the Crash Reporter plugin files under the plugins folder. To get the hook, so that we can add our watcher, I have added a static function over `FlipperBasePlugin`, which if exists, will be called once `REGISTER_PLUGIN` event is dispatched Reviewed By: danielbuechele Differential Revision: D13529035 fbshipit-source-id: 28216b273b4032727859107d8a6751c6465af9ac
2018-12-21 06:58:31 -08:00
parent 780ac863b8
commit 9d4bb45dbc
8 changed files with 282 additions and 192 deletions
--- a/src/dispatcher/iOSDevice.js
+++ b/src/dispatcher/iOSDevice.js
@@ -9,8 +9,6 @@ import type {ChildProcess} from 'child_process';
 import type {Store} from '../reducers/index.js';
 import type Logger from '../fb-stubs/Logger.js';
 import type {DeviceType} from '../devices/BaseDevice';
-import BaseDevice from '../devices/BaseDevice';
-import type {PersistedState} from '../plugins/crash_reporter';
 import {RecurringError} from '../utils/errors';
 import {promisify} from 'util';
 import path from 'path';
@@ -20,12 +18,6 @@ import IOSDevice from '../devices/IOSDevice';
 import iosUtil from '../fb-stubs/iOSContainerUtility';
 import isProduction from '../utils/isProduction.js';
 import GK from '../fb-stubs/GK';
-import fs from 'fs';
-import os from 'os';
-import util from 'util';
-import {setPluginState} from '../reducers/pluginStates.js';
-import {FlipperDevicePlugin, FlipperPlugin} from '../plugin.js';
-import type {State as PluginStatesState} from '../reducers/pluginStates.js';

 type iOSSimulatorDevice = {|
  state: 'Booted' | 'Shutdown' | 'Shutting Down',
@@ -35,12 +27,6 @@ type iOSSimulatorDevice = {|
  udid: string,
 |};

-type Crash = {|
-  callstack: string,
-  reason: string,
-  name: string,
-|};
-
 type IOSDeviceParams = {udid: string, type: DeviceType, name: string};

 const portforwardingClient = isProduction()
@@ -64,156 +50,6 @@ window.addEventListener('beforeunload', () => {
  portForwarders.forEach(process => process.kill());
 });

-export function parseCrashLog(content: string): Crash {
-  const regex = /Exception Type: *[\w]*/;
-  const arr = regex.exec(content);
-  const exceptionString = arr ? arr[0] : '';
-  const exceptionRegex = /[\w]*$/;
-  const tmp = exceptionRegex.exec(exceptionString);
-  const exception =
-    tmp && tmp[0].length ? tmp[0] : 'Cannot figure out the cause';
-  const crash = {
-    callstack: content,
-    name: exception,
-    reason: exception,
-  };
-  return crash;
-}
-export function parsePath(content: string): ?string {
-  const regex = /Path: *[\w\-\/\.\t\ \_\%]*\n/;
-  const arr = regex.exec(content);
-  if (!arr || arr.length <= 0) {
-    return null;
-  }
-  const pathString = arr[0];
-  const pathRegex = /[\w\-\/\.\t\ \_\%]*\n/;
-  const tmp = pathRegex.exec(pathString);
-  if (!tmp || tmp.length == 0) {
-    return null;
-  }
-  const path = tmp[0];
-  return path.trim();
-}
-
-export function getPersistedState(
-  pluginKey: string,
-  persistingPlugin: ?Class<FlipperPlugin<> | FlipperDevicePlugin<>>,
-  pluginStates: PluginStatesState,
-): ?PersistedState {
-  if (!persistingPlugin) {
-    return null;
-  }
-  const persistedState = {
-    ...persistingPlugin.defaultPersistedState,
-    ...pluginStates[pluginKey],
-  };
-  return persistedState;
-}
-
-export function getPluginKey(
-  selectedDevice: ?BaseDevice,
-  pluginID: string,
-): string {
-  return `${selectedDevice?.serial || 'unknown'}#${pluginID}`;
-}
-
-export function getNewPersisitedStateFromCrashLog(
-  persistedState: ?PersistedState,
-  persistingPlugin: Class<FlipperDevicePlugin<> | FlipperPlugin<>>,
-  content: string,
-): ?PersistedState {
-  const crash = parseCrashLog(content);
-  if (!persistingPlugin.persistedStateReducer) {
-    return null;
-  }
-  const newPluginState = persistingPlugin.persistedStateReducer(
-    persistedState,
-    'crash-report',
-    crash,
-  );
-  return newPluginState;
-}
-
-export function shouldShowCrashNotification(
-  baseDevice: ?BaseDevice,
-  content: string,
-): boolean {
-  const appPath = parsePath(content);
-  const serial: string = baseDevice?.serial || 'unknown';
-  if (!appPath || !appPath.includes(serial)) {
-    // Do not show notifications for the app which are not the selected one
-    return false;
-  }
-  return true;
-}
-
-function parseCrashLogAndUpdateState(store: Store, content: string) {
-  if (
-    !shouldShowCrashNotification(
-      store.getState().connections.selectedDevice,
-      content,
-    )
-  ) {
-    return;
-  }
-  const pluginID = 'CrashReporter';
-  const pluginKey = getPluginKey(
-    store.getState().connections.selectedDevice,
-    pluginID,
-  );
-  const persistingPlugin: ?Class<
-    FlipperDevicePlugin<> | FlipperPlugin<>,
-  > = store.getState().plugins.devicePlugins.get('CrashReporter');
-  if (!persistingPlugin) {
-    return;
-  }
-  const pluginStates = store.getState().pluginStates;
-  const persistedState = getPersistedState(
-    pluginKey,
-    persistingPlugin,
-    pluginStates,
-  );
-  const newPluginState = getNewPersisitedStateFromCrashLog(
-    persistedState,
-    persistingPlugin,
-    content,
-  );
-  if (newPluginState && persistedState !== newPluginState) {
-    store.dispatch(
-      setPluginState({
-        pluginKey,
-        state: newPluginState,
-      }),
-    );
-  }
-}
-
-function addFileWatcherForiOSCrashLogs(store: Store, logger: Logger) {
-  const dir = path.join(os.homedir(), 'Library', 'Logs', 'DiagnosticReports');
-  if (!fs.existsSync(dir)) {
-    // Directory doesn't exist
-    return;
-  }
-  fs.watch(dir, (eventType, filename) => {
-    // We just parse the crash logs with extension `.crash`
-    const checkFileExtension = /.crash$/.exec(filename);
-    if (!filename || !checkFileExtension) {
-      return;
-    }
-    fs.readFile(path.join(dir, filename), 'utf8', function(err, data) {
-      if (store.getState().connections.selectedDevice?.os != 'iOS') {
-        // If the selected device is not iOS don't show crash notifications
-        return;
-      }
-      if (err) {
-        console.error(err);
-        return;
-      }
-      parseCrashLogAndUpdateState(store, util.format(data));
-    });
-  });
-}
-
 function queryDevices(store: Store, logger: Logger): Promise<void> {
  const {connections} = store.getState();
  const currentDeviceIDs: Set<string> = new Set(
@@ -295,7 +131,6 @@ export default (store: Store, logger: Logger) => {
  if (process.platform !== 'darwin') {
    return;
  }
-  addFileWatcherForiOSCrashLogs(store, logger);
  queryDevices(store, logger)
    .then(() => {
      const simulatorUpdateInterval = setInterval(() => {
--- a/src/dispatcher/plugins.js
+++ b/src/dispatcher/plugins.js
@@ -23,6 +23,8 @@ import {remote} from 'electron';
 import {GK} from 'flipper';
 import {FlipperBasePlugin} from '../plugin.js';
 import {setupMenuBar} from '../MenuBar.js';
+import {setPluginState} from '../reducers/pluginStates.js';
+import {getPersistedState} from '../utils/pluginUtils.js';

 export type PluginDefinition = {
  name: string,
@@ -54,6 +56,26 @@ export default (store: Store, logger: Logger) => {
  store.dispatch(addFailedPlugins(failedPlugins));
  store.dispatch(registerPlugins(initialPlugins));

+  initialPlugins.forEach(p => {
+    if (p.onRegisterPlugin) {
+      p.onRegisterPlugin(store, (pluginKey: string, newPluginState: any) => {
+        const persistedState = getPersistedState(
+          pluginKey,
+          p,
+          store.getState().pluginStates,
+        );
+        if (newPluginState && newPluginState !== persistedState) {
+          store.dispatch(
+            setPluginState({
+              pluginKey: pluginKey,
+              state: newPluginState,
+            }),
+          );
+        }
+      });
+    }
+  });
+
  let state: ?State = null;
  store.subscribe(() => {
    const newState = store.getState().plugins;
--- a/src/index.js
+++ b/src/index.js
@@ -19,6 +19,9 @@ export * from './fb-stubs/constants.js';
 export * from './utils/createPaste.js';
 export {connect} from 'react-redux';
 export {selectPlugin} from './reducers/connections';
+export {getPluginKey, getPersistedState} from './utils/pluginUtils.js';
+export {default as BaseDevice} from './devices/BaseDevice.js';
+export type {Store} from './reducers/index.js';

 export {
  default as SidebarExtensions,
--- a/src/plugin.js
+++ b/src/plugin.js
@@ -9,6 +9,7 @@ import type {KeyboardActions} from './MenuBar.js';
 import type {App} from './App.js';
 import type Logger from './fb-stubs/Logger.js';
 import type Client from './Client.js';
+import type {Store} from './reducers/index.js';

 import React from 'react';
 import type {Node} from 'react';
@@ -69,6 +70,13 @@ export class FlipperBasePlugin<
  static getActiveNotifications: ?(
    persistedState: PersistedState,
  ) => Array<Notification>;
+  static onRegisterPlugin: ?(
+    store: Store,
+    setPersistedState: (
+      pluginKey: string,
+      newPluginState: ?PersistedState,
+    ) => void,
+  ) => void;
  // forbid instance properties that should be static
  title: empty;
  id: empty;
--- a/src/plugins/crash_reporter/tests/testCrashReporterPlugin.electron.js
+++ b/src/plugins/crash_reporter/tests/testCrashReporterPlugin.electron.js
@@ -4,18 +4,16 @@
 * LICENSE file in the root directory of this source tree.
 * @format
 */
-
-import {
-  parseCrashLog,
-  getPluginKey,
-  getPersistedState,
-  getNewPersisitedStateFromCrashLog,
-  parsePath,
-  shouldShowCrashNotification,
-} from '../../../dispatcher/iOSDevice.js';
 import BaseDevice from '../../../devices/BaseDevice';
 import CrashReporterPlugin from '../../crash_reporter';
 import type {PersistedState, Crash} from '../../crash_reporter';
+import {
+  parseCrashLog,
+  getNewPersisitedStateFromCrashLog,
+  parsePath,
+  shouldShowCrashNotification,
+} from '../../crash_reporter';
+import {getPluginKey, getPersistedState} from '../../../utils/pluginUtils.js';

 function setDefaultPersistedState(defaultState: PersistedState) {
  CrashReporterPlugin.defaultPersistedState = defaultState;
@@ -88,13 +86,22 @@ test('test the parsing of the reason for crash when log is empty', () => {
 });
 test('test the getter of pluginKey with proper input', () => {
  const device = new BaseDevice('serial', 'emulator', 'test device');
-  const pluginKey = getPluginKey(device, 'CrashReporter');
+  const pluginKey = getPluginKey(null, device, 'CrashReporter');
  expect(pluginKey).toEqual('serial#CrashReporter');
 });
 test('test the getter of pluginKey with undefined input', () => {
-  const pluginKey = getPluginKey(undefined, 'CrashReporter');
+  const pluginKey = getPluginKey(null, undefined, 'CrashReporter');
  expect(pluginKey).toEqual('unknown#CrashReporter');
 });
+test('test the getter of pluginKey with defined selected app', () => {
+  const pluginKey = getPluginKey('selectedApp', undefined, 'CrashReporter');
+  expect(pluginKey).toEqual('selectedApp#CrashReporter');
+});
+test('test the getter of pluginKey with defined selected app and defined base device', () => {
+  const device = new BaseDevice('serial', 'emulator', 'test device');
+  const pluginKey = getPluginKey('selectedApp', device, 'CrashReporter');
+  expect(pluginKey).toEqual('selectedApp#CrashReporter');
+});
 test('test defaultPersistedState of CrashReporterPlugin', () => {
  expect(CrashReporterPlugin.defaultPersistedState).toEqual({crashes: []});
 });
@@ -108,7 +115,7 @@ test('test getPersistedState for non-empty defaultPersistedState and undefined p
  setDefaultPersistedState({crashes: [crash]});
  const pluginStates = {};
  const perisistedState = getPersistedState(
-    getPluginKey(null, CrashReporterPlugin.id),
+    getPluginKey(null, null, CrashReporterPlugin.id),
    CrashReporterPlugin,
    pluginStates,
  );
@@ -116,7 +123,7 @@ test('test getPersistedState for non-empty defaultPersistedState and undefined p
 });
 test('test getPersistedState for non-empty defaultPersistedState and defined pluginState', () => {
  const crash = getCrash(0, 'callstack', 'crash0', 'crash0');
-  const pluginKey = getPluginKey(null, CrashReporterPlugin.id);
+  const pluginKey = getPluginKey(null, null, CrashReporterPlugin.id);
  setDefaultPersistedState({crashes: [crash]});
  const pluginStateCrash = getCrash(1, 'callstack', 'crash1', 'crash1');
  const pluginStates = {'unknown#CrashReporter': {crashes: [pluginStateCrash]}};
@@ -129,7 +136,7 @@ test('test getPersistedState for non-empty defaultPersistedState and defined plu
 });
 test('test getNewPersisitedStateFromCrashLog for non-empty defaultPersistedState and defined pluginState', () => {
  const crash = getCrash(0, 'callstack', 'crash0', 'crash0');
-  const pluginKey = getPluginKey(null, CrashReporterPlugin.id);
+  const pluginKey = getPluginKey(null, null, CrashReporterPlugin.id);
  setDefaultPersistedState({crashes: [crash]});
  const pluginStateCrash = getCrash(1, 'callstack', 'crash1', 'crash1');
  const pluginStates = {'unknown#CrashReporter': {crashes: [pluginStateCrash]}};
@@ -153,7 +160,7 @@ test('test getNewPersisitedStateFromCrashLog for non-empty defaultPersistedState
 test('test getNewPersisitedStateFromCrashLog for non-empty defaultPersistedState and undefined pluginState', () => {
  setNotificationID(0);
  const crash = getCrash(0, 'callstack', 'crash0', 'crash0');
-  const pluginKey = getPluginKey(null, CrashReporterPlugin.id);
+  const pluginKey = getPluginKey(null, null, CrashReporterPlugin.id);
  setDefaultPersistedState({crashes: [crash]});
  const pluginStates = {};
  const perisistedState = getPersistedState(
@@ -175,7 +182,7 @@ test('test getNewPersisitedStateFromCrashLog for non-empty defaultPersistedState
 test('test getNewPersisitedStateFromCrashLog for non-empty defaultPersistedState and defined pluginState and improper crash log', () => {
  setNotificationID(0);
  const crash = getCrash(0, 'callstack', 'crash0', 'crash0');
-  const pluginKey = getPluginKey(null, CrashReporterPlugin.id);
+  const pluginKey = getPluginKey(null, null, CrashReporterPlugin.id);
  setDefaultPersistedState({crashes: [crash]});
  const pluginStateCrash = getCrash(1, 'callstack', 'crash1', 'crash1');
  const pluginStates = {'unknown#CrashReporter': {crashes: [pluginStateCrash]}};
--- a/src/plugins/crash_reporter/index.js
+++ b/src/plugins/crash_reporter/index.js
@@ -16,8 +16,17 @@ import {
  ContextMenu,
  clipboard,
  Button,
+  FlipperPlugin,
+  getPluginKey,
+  getPersistedState,
+  BaseDevice,
 } from 'flipper';
+import fs from 'fs';
+import os from 'os';
+import util from 'util';
+import path from 'path';
 import type {Notification} from '../../plugin';
+import type {Store} from 'flipper';

 export type Crash = {|
  notificationID: string,
@@ -26,10 +35,16 @@ export type Crash = {|
  name: string,
 |};

-export type PersistedState = {|
-  crashes: Array<Crash>,
+export type CrashLog = {|
+  callstack: string,
+  reason: string,
+  name: string,
 |};

+export type PersistedState = {
+  crashes: Array<Crash>,
+};
+
 const Title = styled(View)({
  fontWeight: 'bold',
  fontSize: '100%',
@@ -63,7 +78,147 @@ const CallStack = styled('pre')({
  overflow: 'scroll',
 });

-export default class CrashReporterPlugin extends FlipperDevicePlugin {
+export function getNewPersisitedStateFromCrashLog(
+  persistedState: ?PersistedState,
+  persistingPlugin: Class<FlipperDevicePlugin<> | FlipperPlugin<>>,
+  content: string,
+): ?PersistedState {
+  const crash = parseCrashLog(content);
+  if (!persistingPlugin.persistedStateReducer) {
+    return null;
+  }
+  const newPluginState = persistingPlugin.persistedStateReducer(
+    persistedState,
+    'crash-report',
+    crash,
+  );
+  return newPluginState;
+}
+
+export function parseCrashLogAndUpdateState(
+  store: Store,
+  content: string,
+  setPersistedState: (
+    pluginKey: string,
+    newPluginState: ?PersistedState,
+  ) => void,
+) {
+  if (
+    !shouldShowCrashNotification(
+      store.getState().connections.selectedDevice,
+      content,
+    )
+  ) {
+    return;
+  }
+  const pluginID = CrashReporterPlugin.id;
+  const pluginKey = getPluginKey(
+    null,
+    store.getState().connections.selectedDevice,
+    pluginID,
+  );
+  const persistingPlugin: ?Class<
+    FlipperDevicePlugin<> | FlipperPlugin<>,
+  > = store.getState().plugins.devicePlugins.get(CrashReporterPlugin.id);
+  if (!persistingPlugin) {
+    return;
+  }
+  const pluginStates = store.getState().pluginStates;
+  const persistedState = getPersistedState(
+    pluginKey,
+    persistingPlugin,
+    pluginStates,
+  );
+  const newPluginState = getNewPersisitedStateFromCrashLog(
+    persistedState,
+    persistingPlugin,
+    content,
+  );
+  setPersistedState(pluginKey, newPluginState);
+}
+
+export function shouldShowCrashNotification(
+  baseDevice: ?BaseDevice,
+  content: string,
+): boolean {
+  const appPath = parsePath(content);
+  const serial: string = baseDevice?.serial || 'unknown';
+  if (!appPath || !appPath.includes(serial)) {
+    // Do not show notifications for the app which are not the selected one
+    return false;
+  }
+  return true;
+}
+
+export function parseCrashLog(content: string): CrashLog {
+  const regex = /Exception Type: *[\w]*/;
+  const arr = regex.exec(content);
+  const exceptionString = arr ? arr[0] : '';
+  const exceptionRegex = /[\w]*$/;
+  const tmp = exceptionRegex.exec(exceptionString);
+  const exception =
+    tmp && tmp[0].length ? tmp[0] : 'Cannot figure out the cause';
+  const crash = {
+    callstack: content,
+    name: exception,
+    reason: exception,
+  };
+  return crash;
+}
+
+export function parsePath(content: string): ?string {
+  const regex = /Path: *[\w\-\/\.\t\ \_\%]*\n/;
+  const arr = regex.exec(content);
+  if (!arr || arr.length <= 0) {
+    return null;
+  }
+  const pathString = arr[0];
+  const pathRegex = /[\w\-\/\.\t\ \_\%]*\n/;
+  const tmp = pathRegex.exec(pathString);
+  if (!tmp || tmp.length == 0) {
+    return null;
+  }
+  const path = tmp[0];
+  return path.trim();
+}
+
+function addFileWatcherForiOSCrashLogs(
+  store: Store,
+  setPersistedState: (
+    pluginKey: string,
+    newPluginState: ?PersistedState,
+  ) => void,
+) {
+  const dir = path.join(os.homedir(), 'Library', 'Logs', 'DiagnosticReports');
+  if (!fs.existsSync(dir)) {
+    // Directory doesn't exist
+    return;
+  }
+  fs.watch(dir, (eventType, filename) => {
+    // We just parse the crash logs with extension `.crash`
+    const checkFileExtension = /.crash$/.exec(filename);
+    if (!filename || !checkFileExtension) {
+      return;
+    }
+    fs.readFile(path.join(dir, filename), 'utf8', function(err, data) {
+      if (store.getState().connections.selectedDevice?.os != 'iOS') {
+        // If the selected device is not iOS don't show crash notifications
+        return;
+      }
+      if (err) {
+        console.error(err);
+        return;
+      }
+      parseCrashLogAndUpdateState(store, util.format(data), setPersistedState);
+    });
+  });
+}
+
+export default class CrashReporterPlugin extends FlipperDevicePlugin<
+  *,
+  *,
+  PersistedState,
+> {
  static defaultPersistedState = {
    crashes: [],
  };
@@ -98,6 +253,19 @@ export default class CrashReporterPlugin extends FlipperDevicePlugin {
    return persistedState;
  };

+  /*
+   * This function gets called whenever plugin is registered
+   */
+  static onRegisterPlugin = (
+    store: Store,
+    setPersistedState: (
+      pluginKey: string,
+      newPluginState: ?PersistedState,
+    ) => void,
+  ): void => {
+    addFileWatcherForiOSCrashLogs(store, setPersistedState);
+  };
+
  static trimCallStackIfPossible = (callstack: string): string => {
    let regex = /Application Specific Information:/;
    const query = regex.exec(callstack);
@@ -136,7 +304,7 @@ export default class CrashReporterPlugin extends FlipperDevicePlugin {
    if (this.props.deepLinkPayload) {
      const id = this.props.deepLinkPayload;
      const index = this.props.persistedState.crashes.findIndex(elem => {
-        return elem.notificationID === Number(id);
+        return elem.notificationID === id;
      });
      if (index >= 0) {
        crash = this.props.persistedState.crashes[index];
--- a/src/reducers/pluginStates.js
+++ b/src/reducers/pluginStates.js
@@ -29,13 +29,17 @@ export default function reducer(
  action: Action,
 ): State {
  if (action.type === 'SET_PLUGIN_STATE') {
-    return {
-      ...state,
-      [action.payload.pluginKey]: {
-        ...state[action.payload.pluginKey],
-        ...action.payload.state,
-      },
-    };
+    const newPluginState = action.payload.state;
+    if (newPluginState && newPluginState !== state) {
+      return {
+        ...state,
+        [action.payload.pluginKey]: {
+          ...state[action.payload.pluginKey],
+          ...newPluginState,
+        },
+      };
+    }
+    return {...state};
  } else if (action.type === 'CLEAR_PLUGIN_STATE') {
    const {payload} = action;
    return Object.keys(state).reduce((newState, pluginKey) => {
--- a/src/utils/pluginUtils.js
+++ b/src/utils/pluginUtils.js
@@ -0,0 +1,43 @@
+/**
+ * Copyright 2018-present Facebook.
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ * @format
+ */
+import type BaseDevice from '../devices/BaseDevice.js';
+import {FlipperDevicePlugin, FlipperPlugin} from '../plugin.js';
+import type {State as PluginStatesState} from '../reducers/pluginStates.js';
+
+export function getPluginKey(
+  selectedApp: ?string,
+  baseDevice: ?BaseDevice,
+  pluginID: string,
+): string {
+  if (selectedApp) {
+    return `${selectedApp}#${pluginID}`;
+  }
+  if (baseDevice) {
+    // If selected App is not defined, then the plugin is a device plugin
+    return `${baseDevice.serial}#${pluginID}`;
+  }
+  return `unknown#${pluginID}`;
+}
+
+export function getPersistedState<PersistedState>(
+  pluginKey: string,
+  persistingPlugin: ?Class<
+    | FlipperPlugin<*, *, PersistedState>
+    | FlipperDevicePlugin<*, *, PersistedState>,
+  >,
+  pluginStates: PluginStatesState,
+): ?PersistedState {
+  if (!persistingPlugin) {
+    return null;
+  }
+
+  const persistedState: PersistedState = {
+    ...persistingPlugin.defaultPersistedState,
+    ...pluginStates[pluginKey],
+  };
+  return persistedState;
+}