Further decouple server from UI

Summary: Further decoupling of `server/` from the rest of the code base. Also fixed a problem with promise chaining causing promises to create unhandled rejection errors. Reviewed By: passy Differential Revision: D31474919 fbshipit-source-id: 027cccbe9b57d856c94d63c093d39b6cb3e53312
2021-10-12 15:59:44 -07:00
parent 11e7bbf9cf
commit cfd44b592a
13 changed files with 114 additions and 130 deletions
--- a/desktop/app/src/chrome/ScreenCaptureButtons.tsx
+++ b/desktop/app/src/chrome/ScreenCaptureButtons.tsx
@@ -34,6 +34,7 @@ async function openFile(path: string | null) {
  if (fileStat.size <= 8) {
    message.error(
      'Screencap file retrieved from device appears to be corrupt. Your device may not support screen recording. Sometimes restarting your device can help.',
+      0,
    );
    return;
  }
--- a/desktop/app/src/deeplink.tsx
+++ b/desktop/app/src/deeplink.tsx
@@ -58,7 +58,7 @@ export async function handleDeeplink(
        .catch((e: Error) => {
          console.warn('Failed to download Flipper trace', e);
          message.error({
-            duration: null,
+            duration: 0,
            content: 'Failed to download Flipper trace: ' + e,
          });
        })
--- a/desktop/app/src/dispatcher/flipperServer.tsx
+++ b/desktop/app/src/dispatcher/flipperServer.tsx
@@ -10,10 +10,13 @@
 import React from 'react';
 import {State, Store} from '../reducers/index';
 import {Logger} from '../fb-interfaces/Logger';
-import {FlipperServerImpl} from '../server/FlipperServerImpl';
-import {selectClient, selectDevice} from '../reducers/connections';
+import {
+  FlipperServerConfig,
+  FlipperServerImpl,
+} from '../server/FlipperServerImpl';
+import {selectClient} from '../reducers/connections';
 import Client from '../Client';
-import {notification} from 'antd';
+import {message, notification} from 'antd';
 import BaseDevice from '../devices/BaseDevice';
 import {ClientDescription, timeout} from 'flipper-plugin';
 import {reportPlatformFailures} from '../utils/metrics';
@@ -30,8 +33,8 @@ export default async (store: Store, logger: Logger) => {
      enableIOS,
      enablePhysicalIOS,
      serverPorts: store.getState().application.serverPorts,
-    },
-    store,
+      altServerPorts: store.getState().application.altServerPorts,
+    } as FlipperServerConfig,
    logger,
  );

@@ -40,11 +43,13 @@ export default async (store: Store, logger: Logger) => {
    payload: server,
  });

-  server.on('notification', (notif) => {
+  server.on('notification', ({type, title, description}) => {
+    console.warn(`[$type] ${title}: ${description}`);
    notification.open({
-      message: notif.title,
-      description: notif.description,
-      type: notif.type,
+      message: title,
+      description: description,
+      type: type,
+      duration: 0,
    });
  });

@@ -114,6 +119,13 @@ export default async (store: Store, logger: Logger) => {
    // N.B.: note that we don't remove the device, we keep it in offline
  });

+  server.on('client-setup', (client) => {
+    store.dispatch({
+      type: 'START_CLIENT_SETUP',
+      payload: client,
+    });
+  });
+
  server.on('client-connected', (payload: ClientDescription) =>
    handleClientConnected(server, store, logger, payload),
  );
@@ -178,9 +190,28 @@ export async function handleClientConnected(
    });
  }

+  console.log(
+    `[conn] Searching matching device ${query.device_id} for client ${query.app}...`,
+  );
  const device =
    getDeviceBySerial(store.getState(), query.device_id) ??
-    (await findDeviceForConnection(store, query.app, query.device_id));
+    (await findDeviceForConnection(store, query.app, query.device_id).catch(
+      (e) => {
+        console.error(
+          `[conn] Failed to find device '${query.device_id}' while connection app '${query.app}'`,
+          e,
+        );
+        notification.error({
+          message: 'Connection failed',
+          description: `Failed to find device '${query.device_id}' while trying to connect app '${query.app}'`,
+          duration: 0,
+        });
+      },
+    ));
+
+  if (!device) {
+    return;
+  }

  const client = new Client(
    id,
@@ -218,6 +249,7 @@ export async function handleClientConnected(
    client.init(),
    `[conn] Failed to initialize client ${query.app} on ${query.device_id} in a timely manner`,
  );
+  console.log(`[conn] ${query.app} on ${query.device_id} connected and ready.`);
 }

 function getDeviceBySerial(
@@ -242,12 +274,11 @@ async function findDeviceForConnection(

      const timeout = setTimeout(() => {
        unsubscribe();
-        const error = `Timed out waiting for device ${serial} for client ${clientId}`;
-        console.error(
-          '[conn] Unable to find device for connection. Error:',
-          error,
+        reject(
+          new Error(
+            `Timed out waiting for device ${serial} for client ${clientId}`,
+          ),
        );
-        reject(error);
      }, 15000);
      unsubscribe = sideEffect(
        store,
--- a/desktop/app/src/reducers/connections.tsx
+++ b/desktop/app/src/reducers/connections.tsx
@@ -12,7 +12,7 @@ import {produce} from 'immer';

 import type BaseDevice from '../devices/BaseDevice';
 import type Client from '../Client';
-import type {UninitializedClient} from '../server/UninitializedClient';
+import type {UninitializedClient} from 'flipper-plugin';
 import {performance} from 'perf_hooks';
 import type {Actions} from '.';
 import {WelcomeScreenStaticView} from '../sandy-chrome/WelcomeScreen';
--- a/desktop/app/src/server/FlipperServerImpl.tsx
+++ b/desktop/app/src/server/FlipperServerImpl.tsx
@@ -8,15 +8,9 @@
 */

 import EventEmitter from 'events';
-import {Store} from '../reducers/index';
 import {Logger} from '../fb-interfaces/Logger';
 import ServerController from './comms/ServerController';
-import {UninitializedClient} from './UninitializedClient';
-import {addErrorNotification} from '../reducers/notifications';
 import {CertificateExchangeMedium} from './utils/CertificateProvider';
-import {isLoggedIn} from '../fb-stubs/user';
-import React from 'react';
-import {Typography} from 'antd';
 import {ServerPorts} from '../reducers/application';
 import {AndroidDeviceManager} from './devices/android/androidDeviceManager';
 import {IOSDeviceManager} from './devices/ios/iOSDeviceManager';
@@ -27,11 +21,11 @@ import {
  FlipperServerState,
  FlipperServerCommands,
  FlipperServer,
+  UninitializedClient,
 } from 'flipper-plugin';
 import {ServerDevice} from './devices/ServerDevice';
 import {Base64} from 'js-base64';
 import MetroDevice from './devices/metro/MetroDevice';
-import {showLoginDialog} from '../chrome/fb-stubs/SignInSheet';

 export interface FlipperServerConfig {
  enableAndroid: boolean;
@@ -40,6 +34,7 @@ export interface FlipperServerConfig {
  idbPath: string;
  enablePhysicalIOS: boolean;
  serverPorts: ServerPorts;
+  altServerPorts: ServerPorts;
 }

 // defaultConfig should be used for testing only, and disables by default all features
@@ -53,6 +48,10 @@ const defaultConfig: FlipperServerConfig = {
    insecure: -1,
    secure: -1,
  },
+  altServerPorts: {
+    insecure: -1,
+    secure: -1,
+  },
 };

 /**
@@ -75,13 +74,7 @@ export class FlipperServerImpl implements FlipperServer {
  android: AndroidDeviceManager;
  ios: IOSDeviceManager;

-  // TODO: remove store argument
-  constructor(
-    config: Partial<FlipperServerConfig>,
-    /** @deprecated remove! */
-    public store: Store,
-    public logger: Logger,
-  ) {
+  constructor(config: Partial<FlipperServerConfig>, public logger: Logger) {
    this.config = {...defaultConfig, ...config};
    const server = (this.server = new ServerController(this));
    this.android = new AndroidDeviceManager(this);
@@ -92,22 +85,17 @@ export class FlipperServerImpl implements FlipperServer {
    });

    server.addListener('start-client-setup', (client: UninitializedClient) => {
-      this.store.dispatch({
-        type: 'START_CLIENT_SETUP',
-        payload: client,
-      });
+      this.emit('client-setup', client);
    });

    server.addListener(
      'client-setup-error',
      ({client, error}: {client: UninitializedClient; error: Error}) => {
-        this.store.dispatch(
-          addErrorNotification(
-            `Connection to '${client.appName}' on '${client.deviceName}' failed`,
-            'Failed to start client connection',
-            error,
-          ),
-        );
+        this.emit('notification', {
+          title: `Connection to '${client.appName}' on '${client.deviceName}' failed`,
+          description: `Failed to start client connection: ${error}`,
+          type: 'error',
+        });
      },
    );

@@ -121,35 +109,14 @@ export class FlipperServerImpl implements FlipperServer {
        medium: CertificateExchangeMedium;
        deviceID: string;
      }) => {
-        this.store.dispatch(
-          addErrorNotification(
-            `Timed out establishing connection with "${client.appName}" on "${client.deviceName}".`,
-            medium === 'WWW' ? (
-              <>
-                Verify that both your computer and mobile device are on
-                Lighthouse/VPN{' '}
-                {!isLoggedIn().get() && (
-                  <>
-                    and{' '}
-                    <Typography.Link
-                      onClick={() => {
-                        showLoginDialog();
-                      }}>
-                      log in to Facebook Intern
-                    </Typography.Link>
-                  </>
-                )}{' '}
-                so they can exchange certificates.{' '}
-                <Typography.Link href="https://www.internalfb.com/intern/wiki/Ops/Network/Enterprise_Network_Engineering/ene_wlra/VPN_Help/Vpn/mobile/">
-                  Check this link
-                </Typography.Link>{' '}
-                on how to enable VPN on mobile device.
-              </>
-            ) : (
-              'Verify that your client is connected to Flipper and that there is no error related to idb.'
-            ),
-          ),
-        );
+        this.emit('notification', {
+          type: 'error',
+          title: `Timed out establishing connection with "${client.appName}" on "${client.deviceName}".`,
+          description:
+            medium === 'WWW'
+              ? `Verify that both your computer and mobile device are on Lighthouse/VPN that you are logged in to Facebook Intern so that certificates can be exhanged. See: https://www.internalfb.com/intern/wiki/Ops/Network/Enterprise_Network_Engineering/ene_wlra/VPN_Help/Vpn/mobile/`
+              : 'Verify that your client is connected to Flipper and that there is no error related to idb.',
+        });
      },
    );
  }
--- a/desktop/app/src/server/UninitializedClient.tsx
+++ b/desktop/app/src/server/UninitializedClient.tsx
@@ -1,14 +0,0 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- * @format
- */
-
-export type UninitializedClient = {
-  os: string;
-  deviceName: string;
-  appName: string;
-};
--- a/desktop/app/src/server/comms/ServerController.tsx
+++ b/desktop/app/src/server/comms/ServerController.tsx
@@ -10,10 +10,9 @@
 import {CertificateExchangeMedium} from '../utils/CertificateProvider';
 import {Logger} from '../../fb-interfaces/Logger';
 import {ClientDescription, ClientQuery} from 'flipper-plugin';
-import {Store} from '../../reducers/index';
 import CertificateProvider from '../utils/CertificateProvider';
 import {ClientConnection, ConnectionStatus} from './ClientConnection';
-import {UninitializedClient} from '../UninitializedClient';
+import {UninitializedClient} from 'flipper-plugin';
 import {reportPlatformFailures} from '../../utils/metrics';
 import {EventEmitter} from 'events';
 import invariant from 'invariant';
@@ -82,7 +81,7 @@ class ServerController extends EventEmitter implements ServerEventsListener {
    this.certificateProvider = new CertificateProvider(
      this,
      this.logger,
-      this.store.getState().settingsState,
+      this.flipperServer.config,
    );
    this.connectionTracker = new ConnectionTracker(this.logger);
    this.secureServer = null;
@@ -104,13 +103,6 @@ class ServerController extends EventEmitter implements ServerEventsListener {
    return this.flipperServer.logger;
  }

-  /**
-   * @deprecated
-   */
-  get store(): Store {
-    return this.flipperServer.store;
-  }
-
  /**
   * Loads the secure server configuration and starts any necessary servers.
   * Initialisation is complete once the initialized promise is fullfilled at
@@ -120,7 +112,7 @@ class ServerController extends EventEmitter implements ServerEventsListener {
    if (isTest()) {
      throw new Error('Spawing new server is not supported in test');
    }
-    const {insecure, secure} = this.store.getState().application.serverPorts;
+    const {insecure, secure} = this.flipperServer.config.serverPorts;

    this.initialized = this.certificateProvider
      .loadSecureServerConfig()
@@ -128,8 +120,7 @@ class ServerController extends EventEmitter implements ServerEventsListener {
        console.info('[conn] secure server listening at port: ', secure);
        this.secureServer = createServer(secure, this, options);
        if (GK.get('flipper_websocket_server')) {
-          const {secure: altSecure} =
-            this.store.getState().application.altServerPorts;
+          const {secure: altSecure} = this.flipperServer.config.altServerPorts;
          console.info(
            '[conn] secure server (ws) listening at port: ',
            altSecure,
@@ -147,7 +138,7 @@ class ServerController extends EventEmitter implements ServerEventsListener {
        this.insecureServer = createServer(insecure, this);
        if (GK.get('flipper_websocket_server')) {
          const {insecure: altInsecure} =
-            this.store.getState().application.altServerPorts;
+            this.flipperServer.config.altServerPorts;
          console.info(
            '[conn] insecure server (ws) listening at port: ',
            altInsecure,
--- a/desktop/app/src/server/comms/ServerRSocket.tsx
+++ b/desktop/app/src/server/comms/ServerRSocket.tsx
@@ -173,7 +173,7 @@ class ServerRSocket extends ServerAdapter {
    client
      .then((client) => {
        console.info(
-          `[conn] Client created: ${clientQuery.app} on ${clientQuery.device_id}. Medium ${clientQuery.medium}. CSR: ${clientQuery.csr_path}`,
+          `[conn] Client connected: ${clientQuery.app} on ${clientQuery.device_id}. Medium ${clientQuery.medium}. CSR: ${clientQuery.csr_path}`,
        );
        resolvedClient = client;
      })
--- a/desktop/app/src/server/comms/ServerWebSocketBrowser.tsx
+++ b/desktop/app/src/server/comms/ServerWebSocketBrowser.tsx
@@ -122,7 +122,7 @@ class ServerWebSocketBrowser extends ServerWebSocketBase {
          client
            .then((client) => {
              console.info(
-                `[conn] Client created: ${clientQuery.app} on ${clientQuery.device_id}.`,
+                `[conn] Client connected: ${clientQuery.app} on ${clientQuery.device_id}.`,
              );
              resolvedClient = client;
            })
--- a/desktop/app/src/server/devices/ios/tests/iOSDevice.node.tsx
+++ b/desktop/app/src/server/devices/ios/tests/iOSDevice.node.tsx
@@ -59,7 +59,7 @@ test('test parseXcodeFromCoreSimPath from standard locations', () => {
 });

 test('test getAllPromisesForQueryingDevices when xcode detected', () => {
-  const flipperServer = new FlipperServerImpl({}, mockStore, getInstance());
+  const flipperServer = new FlipperServerImpl({}, getInstance());
  flipperServer.ios.iosBridge = {} as IOSBridge;
  const promises = flipperServer.ios.getAllPromisesForQueryingDevices(
    true,
@@ -69,7 +69,7 @@ test('test getAllPromisesForQueryingDevices when xcode detected', () => {
 });

 test('test getAllPromisesForQueryingDevices when xcode is not detected', () => {
-  const flipperServer = new FlipperServerImpl({}, mockStore, getInstance());
+  const flipperServer = new FlipperServerImpl({}, getInstance());
  flipperServer.ios.iosBridge = {} as IOSBridge;
  const promises = flipperServer.ios.getAllPromisesForQueryingDevices(
    false,
@@ -79,7 +79,7 @@ test('test getAllPromisesForQueryingDevices when xcode is not detected', () => {
 });

 test('test getAllPromisesForQueryingDevices when xcode and idb are both unavailable', () => {
-  const flipperServer = new FlipperServerImpl({}, mockStore, getInstance());
+  const flipperServer = new FlipperServerImpl({}, getInstance());
  flipperServer.ios.iosBridge = {} as IOSBridge;
  const promises = flipperServer.ios.getAllPromisesForQueryingDevices(
    false,
@@ -89,7 +89,7 @@ test('test getAllPromisesForQueryingDevices when xcode and idb are both unavaila
 });

 test('test getAllPromisesForQueryingDevices when both idb and xcode are available', () => {
-  const flipperServer = new FlipperServerImpl({}, mockStore, getInstance());
+  const flipperServer = new FlipperServerImpl({}, getInstance());
  flipperServer.ios.iosBridge = {} as IOSBridge;
  const promises = flipperServer.ios.getAllPromisesForQueryingDevices(
    true,
--- a/desktop/app/src/server/utils/CertificateProvider.tsx
+++ b/desktop/app/src/server/utils/CertificateProvider.tsx
@@ -118,12 +118,12 @@ export default class CertificateProvider {
      ? (getAdbClient(config).catch((e) => {
          // make sure initialization failure is already logged
          const msg =
-            'Failed to initialize ADB. Please disabled Android support in the settings, or configure a correct path';
-          message.error({
-            duration: 10,
-            content: msg + e,
+            'Failed to initialize ADB. Please disable Android support in settings, or configure a correct path';
+          server.flipperServer.emit('notification', {
+            type: 'error',
+            title: 'Failed to initialise ADB',
+            description: msg,
          });
-          console.warn(msg, e);
          this._adb = undefined; // no adb client available
        }) as Promise<ADBClient>)
      : undefined;
--- a/desktop/app/src/utils/metrics.tsx
+++ b/desktop/app/src/utils/metrics.tsx
@@ -32,12 +32,13 @@ export function reportPlatformFailures<T>(
  promise: Promise<T>,
  name: string,
 ): Promise<T> {
-  return promise.then(
-    (fulfilledValue) => {
+  return new Promise<T>((resolve, reject) => {
+    promise
+      .then((fulfilledValue) => {
        logPlatformSuccessRate(name, {kind: 'success'});
-      return fulfilledValue;
-    },
-    (rejectionReason) => {
+        resolve(fulfilledValue);
+      })
+      .catch((rejectionReason) => {
        if (rejectionReason instanceof CancelledPromiseError) {
          logPlatformSuccessRate(name, {
            kind: 'cancelled',
@@ -49,9 +50,9 @@ export function reportPlatformFailures<T>(
            error: rejectionReason,
          });
        }
-      return Promise.reject(rejectionReason);
-    },
-  );
+        reject(rejectionReason);
+      });
+  });
 }

 /*
--- a/desktop/flipper-plugin/src/types/server-types.tsx
+++ b/desktop/flipper-plugin/src/types/server-types.tsx
@@ -40,6 +40,12 @@ export type DeviceDescription = {
  readonly sdkVersion?: string;
 };

+export type UninitializedClient = {
+  os: string;
+  deviceName: string;
+  appName: string;
+};
+
 export type ClientQuery = {
  readonly app: string;
  readonly os: DeviceOS;
@@ -79,6 +85,7 @@ export type FlipperServerEvents = {
    serial: string;
    entry: DeviceLogEntry;
  };
+  'client-setup': UninitializedClient;
  'client-connected': ClientDescription;
  'client-disconnected': {id: string};
  'client-message': {