Simplify bridge types

Summary: This turns the bridge type into a simpler struct with always-present methods so you don't need to add additional null check to the calling logic which are hard to deal with.

Reviewed By: mweststrate

Differential Revision: D30248628

fbshipit-source-id: cdaee44efcbb19dcbb301099b4a7d0eb0c350e67

desktop/app/src/devices/IOSDevice.tsx

@@ -14,7 +14,10 @@ import JSONStream from 'JSONStream';
 import {Transform} from 'stream';
 import {exec} from 'promisify-child-process';
 import {default as promiseTimeout} from '../utils/promiseTimeout';
-import {IOSBridge} from '../utils/IOSBridge';
+import {
+  ERR_PHYSICAL_DEVICE_LOGS_WITHOUT_IDB,
+  IOSBridge,
+} from '../utils/IOSBridge';
 import split2 from 'split2';
 
 type IOSLogLevel = 'Default' | 'Info' | 'Debug' | 'Error' | 'Fault';
@@ -63,8 +66,7 @@ export default class IOSDevice extends BaseDevice {
     if (!this.connected.get()) {
       return Buffer.from([]);
     }
-    // HACK: Will restructure the types to allow for the ! to be removed.
-    return await this.iOSBridge.screenshot!(this.serial);
+    return await this.iOSBridge.screenshot(this.serial);
   }
 
   navigateToLocation(location: string) {
@@ -86,14 +88,18 @@ export default class IOSDevice extends BaseDevice {
       return;
     }
 
-    const logListener = iOSBridge.startLogListener;
-    if (
-      !this.log &&
-      logListener &&
-      (this.deviceType === 'emulator' ||
-        (this.deviceType === 'physical' && iOSBridge.idbAvailable))
-    ) {
-      this.log = logListener(this.serial, this.deviceType);
+    if (!this.log) {
+      try {
+        this.log = iOSBridge.startLogListener(this.serial, this.deviceType);
+      } catch (e) {
+        if (e.message === ERR_PHYSICAL_DEVICE_LOGS_WITHOUT_IDB) {
+          console.warn(e);
+        } else {
+          console.error('Failed to initialise device logs:', e);
+          this.startLogListener(iOSBridge, retries - 1);
+        }
+        return;
+      }
       this.log.on('error', (err: Error) => {
         console.error('iOS log tailer error', err);
       });

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

@@ -14,6 +14,7 @@ import {
 import configureStore from 'redux-mock-store';
 import {State, createRootReducer} from '../../reducers/index';
 import {getInstance} from '../../fb-stubs/Logger';
+import {IOSBridge} from '../../utils/IOSBridge';
 
 const mockStore = configureStore<State, {}>([])(
   createRootReducer()(undefined, {type: 'INIT'}),
@@ -64,9 +65,7 @@ test('test getAllPromisesForQueryingDevices when xcode detected', () => {
   const promises = getAllPromisesForQueryingDevices(
     mockStore,
     logger,
-    {
-      idbAvailable: false,
-    },
+    {} as IOSBridge,
     true,
   );
   expect(promises.length).toEqual(3);
@@ -76,9 +75,7 @@ test('test getAllPromisesForQueryingDevices when xcode is not detected', () => {
   const promises = getAllPromisesForQueryingDevices(
     mockStore,
     logger,
-    {
-      idbAvailable: true,
-    },
+    {} as IOSBridge,
     false,
   );
   expect(promises.length).toEqual(1);

desktop/app/src/dispatcher/iOSDevice.tsx

@@ -21,7 +21,11 @@ import IOSDevice from '../devices/IOSDevice';
 import {addErrorNotification} from '../reducers/notifications';
 import {getStaticPath} from '../utils/pathUtils';
 import {destroyDevice} from '../reducers/connections';
-import {IOSBridge, makeIOSBridge} from '../utils/IOSBridge';
+import {
+  ERR_NO_IDB_OR_XCODE_AVAILABLE,
+  IOSBridge,
+  makeIOSBridge,
+} from '../utils/IOSBridge';
 
 type iOSSimulatorDevice = {
   state: 'Booted' | 'Shutdown' | 'Shutting Down';
@@ -312,21 +316,29 @@ export default (store: Store, logger: Logger) => {
   }
   iosUtil
     .isXcodeDetected()
-    .then(
-      (isDetected) => {
+    .then(async (isDetected) => {
       store.dispatch(setXcodeDetected(isDetected));
       if (store.getState().settingsState.enablePhysicalIOS) {
         startDevicePortForwarders();
       }
-        return makeIOSBridge(
+      try {
+        // Awaiting the promise here to trigger immediate error handling.
+        return await makeIOSBridge(
           store.getState().settingsState.idbPath,
           isDetected,
         );
-      },
-      (err) => {
+      } catch (err) {
+        // This case is expected if both Xcode and idb are missing.
+        if (err.message === ERR_NO_IDB_OR_XCODE_AVAILABLE) {
+          console.warn(
+            'Failed to init iOS device. You may want to disable iOS support in the settings.',
+            err,
+          );
+        } else {
           console.error('Failed to initialize iOS dispatcher:', err);
-      },
-    )
+        }
+      }
+    })
     .then(
       (iosBridge) => iosBridge && queryDevicesForever(store, logger, iosBridge),
     )

desktop/app/src/utils/IOSBridge.tsx

@@ -15,13 +15,18 @@ import path from 'path';
 import {exec} from 'promisify-child-process';
 import {getAppTempPath} from '../utils/pathUtils';
 
+export const ERR_NO_IDB_OR_XCODE_AVAILABLE =
+  'Neither Xcode nor idb available. Cannot provide iOS device functionality.';
+
+export const ERR_PHYSICAL_DEVICE_LOGS_WITHOUT_IDB =
+  'Cannot provide logs from a physical device without idb.';
+
 export interface IOSBridge {
-  idbAvailable: boolean;
-  startLogListener?: (
+  startLogListener: (
     udid: string,
     deviceType: DeviceType,
   ) => child_process.ChildProcessWithoutNullStreams;
-  screenshot?: (serial: string) => Promise<Buffer>;
+  screenshot: (serial: string) => Promise<Buffer>;
 }
 
 async function isAvailable(idbPath: string): Promise<boolean> {
@@ -65,6 +70,9 @@ export function idbStartLogListener(
 }
 
 export function xcrunStartLogListener(udid: string, deviceType: DeviceType) {
+  if (deviceType === 'physical') {
+    throw new Error(ERR_PHYSICAL_DEVICE_LOGS_WITHOUT_IDB);
+  }
   const deviceSetPath = process.env.DEVICE_SET_PATH
     ? ['--set', process.env.DEVICE_SET_PATH]
     : [];
@@ -121,7 +129,6 @@ export async function makeIOSBridge(
   // prefer idb
   if (await isAvailableFn(idbPath)) {
     return {
-      idbAvailable: true,
       startLogListener: idbStartLogListener.bind(null, idbPath),
       screenshot: idbScreenshot,
     };
@@ -130,13 +137,10 @@ export async function makeIOSBridge(
   // no idb, if it's a simulator and xcode is available, we can use xcrun
   if (isXcodeDetected) {
     return {
-      idbAvailable: false,
       startLogListener: xcrunStartLogListener,
       screenshot: xcrunScreenshot,
     };
   }
-  // no idb, and not a simulator, we can't log this device
-  return {
-    idbAvailable: false,
-  };
+
+  throw new Error(ERR_NO_IDB_OR_XCODE_AVAILABLE);
 }

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

@@ -20,9 +20,8 @@ const exec = mocked(promisifyChildProcess.exec);
 
 test('uses xcrun with no idb when xcode is detected', async () => {
   const ib = await makeIOSBridge('', true);
-  expect(ib.startLogListener).toBeDefined();
 
-  ib.startLogListener!('deadbeef', 'emulator');
+  ib.startLogListener('deadbeef', 'emulator');
 
   expect(spawn).toHaveBeenCalledWith(
     'xcrun',
@@ -45,9 +44,8 @@ test('uses xcrun with no idb when xcode is detected', async () => {
 
 test('uses idb when present and xcode detected', async () => {
   const ib = await makeIOSBridge('/usr/local/bin/idb', true, async (_) => true);
-  expect(ib.startLogListener).toBeDefined();
 
-  ib.startLogListener!('deadbeef', 'emulator');
+  ib.startLogListener('deadbeef', 'emulator');
 
   expect(spawn).toHaveBeenCalledWith(
     '/usr/local/bin/idb',
@@ -69,9 +67,8 @@ test('uses idb when present and xcode detected', async () => {
 
 test('uses idb when present and xcode detected and physical device connected', async () => {
   const ib = await makeIOSBridge('/usr/local/bin/idb', true, async (_) => true);
-  expect(ib.startLogListener).toBeDefined();
 
-  ib.startLogListener!('deadbeef', 'physical');
+  ib.startLogListener('deadbeef', 'physical');
 
   expect(spawn).toHaveBeenCalledWith(
     '/usr/local/bin/idb',
@@ -88,19 +85,19 @@ test('uses idb when present and xcode detected and physical device connected', a
 
 test("without idb physical devices can't log", async () => {
   const ib = await makeIOSBridge('', true);
-  expect(ib.idbAvailable).toBeFalsy();
   expect(ib.startLogListener).toBeDefined(); // since we have xcode
 });
 
-test('uses no log listener when xcode is not detected', async () => {
-  const ib = await makeIOSBridge('', false);
-  expect(ib.startLogListener).toBeUndefined();
+test('throws if no iOS support', async () => {
+  await expect(makeIOSBridge('', false)).rejects.toThrow(
+    'Neither Xcode nor idb available. Cannot provide iOS device functionality.',
+  );
 });
 
 test('uses xcrun to take screenshots with no idb when xcode is detected', async () => {
   const ib = await makeIOSBridge('', true);
 
-  ib.screenshot!('deadbeef');
+  ib.screenshot('deadbeef');
 
   expect(exec).toHaveBeenCalledWith(
     'xcrun simctl io deadbeef screenshot /temp/00000000-0000-0000-0000-000000000000.png',
@@ -110,7 +107,7 @@ test('uses xcrun to take screenshots with no idb when xcode is detected', async
 test('uses idb to take screenshots when available', async () => {
   const ib = await makeIOSBridge('/usr/local/bin/idb', true, async (_) => true);
 
-  ib.screenshot!('deadbeef');
+  ib.screenshot('deadbeef');
 
   expect(exec).toHaveBeenCalledWith(
     'idb screenshot --udid deadbeef /temp/00000000-0000-0000-0000-000000000000.png',