Processed some earlier review comments

Summary: Per title, processed some pending review comments made earlier in this stack

Reviewed By: aigoncharov

Differential Revision: D32916920

fbshipit-source-id: 01db85883596b5c85b77efc9cddadeac23cc4ef5

desktop/flipper-server-core/src/FlipperServerImpl.tsx

@@ -33,7 +33,7 @@ import {launchEmulator} from './devices/android/AndroidDevice';
 import {setFlipperServerConfig} from './FlipperServerConfig';
 import {saveSettings} from './utils/settings';
 import {saveLauncherSettings} from './utils/launcherSettings';
-import {KeytarManager} from './utils/keytar';
+import {KeytarManager, KeytarModule} from './utils/keytar';
 import {PluginManager} from './plugins/PluginManager';
 import {runHealthcheck, getHealthChecks} from './utils/runHealthchecks';
 import {openFile} from './utils/openFile';
@@ -69,15 +69,16 @@ export class FlipperServerImpl implements FlipperServer {
   constructor(
     public config: FlipperServerConfig,
     public logger: Logger,
-    keytarModule?: any,
+    keytarModule?: KeytarModule,
   ) {
     setFlipperServerConfig(config);
     const server = (this.server = new ServerController(this));
     this.android = new AndroidDeviceManager(this);
     this.ios = new IOSDeviceManager(this);
     this.keytarManager = new KeytarManager(keytarModule);
-    // TODO: given flipper-dump, it might make more sense to have the plugin command
+    // given flipper-dump, it might make more sense to have the plugin command
-    // handled by moved to flipper-server & app, but let's keep things simple for now
+    // handling (like download, install, etc) moved to flipper-server & app,
+    // but let's keep things simple for now
     this.pluginManager = new PluginManager();
 
     server.addListener('error', (err) => {

desktop/flipper-server-core/src/utils/keytar.tsx

@@ -10,12 +10,14 @@
 import os from 'os';
 import {UserNotSignedInError} from 'flipper-common';
 
-export class KeytarManager {
+export type KeytarModule = {
-  keytar: any;
+  getPassword(service: string, username: string): string;
+  deletePassword(service: string, username: string): void;
+  setPassword(service: string, username: string, password: string): void;
+};
 
-  constructor(keytarModule: any) {
+export class KeytarManager {
-    this.keytar = keytarModule;
+  constructor(private keytar: KeytarModule | undefined) {}
-  }
 
   public async writeKeychain(service: string, password: string): Promise<void> {
     if (this.keytar == null) {
@@ -27,7 +29,7 @@ export class KeytarManager {
   }
 
   public async unsetKeychain(service: string): Promise<void> {
-    await this.keytar.deletePassword(service, os.userInfo().username);
+    await this.keytar?.deletePassword(service, os.userInfo().username);
   }
 
   public async retrieveToken(service: string): Promise<string> {

desktop/flipper-server-core/src/utils/launcherSettings.tsx

@@ -14,6 +14,9 @@ import fs from 'fs-extra';
 import TOML, {JsonMap} from '@iarna/toml';
 import {LauncherSettings, ReleaseChannel} from 'flipper-common';
 
+// There is some disagreement among the XDG Base Directory implementations
+// whether to use ~/Library/Preferences or ~/.config on MacOS. The Launcher
+// expects the former, whereas `xdg-basedir` implements the latter.
 export function xdgConfigDir() {
   return os.platform() === 'darwin'
     ? path.join(os.homedir(), 'Library', 'Preferences')
@@ -28,9 +31,6 @@ export function launcherConfigDir() {
 }
 
 function getLauncherSettingsFile(): string {
-  // There is some disagreement among the XDG Base Directory implementations
-  // whether to use ~/Library/Preferences or ~/.config on MacOS. The Launcher
-  // expects the former, whereas `xdg-basedir` implements the latter.
   return path.resolve(launcherConfigDir(), 'flipper-launcher.toml');
 }
 

desktop/flipper-server/src/startSocketServer.tsx

@@ -46,5 +46,11 @@ export function startSocketServer(
       connected = false;
       flipperServer.offAny(onServerEvent);
     });
+
+    client.on('error', (e) => {
+      console.error(chalk.red(`Socket error ${client.id}`), e);
+      connected = false;
+      flipperServer.offAny(onServerEvent);
+    });
   });
 }

desktop/flipper-server/src/startWebServerDev.tsx

@@ -119,7 +119,7 @@ async function startMetroServer(
       )
     ).flat(),
   );
-  // console.log('Source dirs\n\t' + watchFolders.join('\n\t'));
+
   const baseConfig = await Metro.loadConfig();
   const config = Object.assign({}, baseConfig, {
     projectRoot: rootDir,

desktop/static/index.web.dev.html

@@ -114,7 +114,7 @@
           script.src = window.flipperConfig.entryPoint;
 
           script.onerror = (e) => {
-            openError('Script failure. Check Chrome console for more info. ');
+            openError('Script failure. Check Chrome console for more info.');
           };
 
           document.body.appendChild(script);