From f0526ffc3fabe4b77a33c9c9cea75afd2594ef0a Mon Sep 17 00:00:00 2001
From: Gleb Bahmutov <gleb.bahmutov@gmail.com>
Date: Wed, 24 May 2017 22:26:58 -0400
Subject: [PATCH] Refactor browser detection (#91)

* refactor browser detection to have single facade function for each platform

* refactor found browser

* update to tslint@5.3.2

* short detection code without duplicates

* deleted obsolete individual browser files

* launcher: detect and grab custom alias google-chrome-stable

* keep same browser name, and set human name as displayName property

* if mdfind fails, try using /Applications path to detect

* remove duplicate code

* detect and remove duplicate browser detections

* refactor for readability

* change chromium version property
---
 packages/launcher/lib/browsers.ts        | 47 ++++++++++++++--
 packages/launcher/lib/darwin/canary.ts   | 22 --------
 packages/launcher/lib/darwin/chrome.ts   | 28 ----------
 packages/launcher/lib/darwin/chromium.ts | 25 ---------
 packages/launcher/lib/darwin/index.ts    | 45 ++++++++++++---
 packages/launcher/lib/darwin/util.ts     | 46 +++++++++++++++-
 packages/launcher/lib/detect.ts          | 70 ++++++++----------------
 packages/launcher/lib/linux/index.ts     | 54 ++++++++++--------
 packages/launcher/lib/types.ts           | 19 ++++++-
 packages/launcher/package.json           |  2 +-
 10 files changed, 195 insertions(+), 163 deletions(-)
 delete mode 100644 packages/launcher/lib/darwin/canary.ts
 delete mode 100644 packages/launcher/lib/darwin/chrome.ts
 delete mode 100644 packages/launcher/lib/darwin/chromium.ts

diff --git a/packages/launcher/lib/browsers.ts b/packages/launcher/lib/browsers.ts
index 7f328648bb..8c3a550186 100644
--- a/packages/launcher/lib/browsers.ts
+++ b/packages/launcher/lib/browsers.ts
@@ -1,12 +1,7 @@
 import {log} from './log'
 import {find, map} from 'lodash'
 import cp = require('child_process')
-import {BrowserNotFoundError} from './types'
-
-type FoundBrowser = {
-  name: string,
-  path?: string
-}
+import {Browser, FoundBrowser, BrowserNotFoundError} from './types'
 
 const browserNotFoundErr = (browsers: FoundBrowser[], name: string): BrowserNotFoundError => {
   const available = map(browsers, 'name').join(', ')
@@ -17,6 +12,46 @@ const browserNotFoundErr = (browsers: FoundBrowser[], name: string): BrowserNotF
   return err
 }
 
+const googleChromeStable: Browser = {
+  name: 'Google Chrome Stable',
+  versionRegex: /Google Chrome (\S+)/,
+  profile: true,
+  binary: 'google-chrome-stable'
+}
+
+const googleChromeAlias: Browser = {
+  name: 'Google Chrome',
+  versionRegex: /Google Chrome (\S+)/,
+  profile: true,
+  binary: 'chrome'
+}
+
+/** list of all browsers we can detect and use */
+export const browsers: Browser[] = [
+  {
+    name: 'chrome',
+    displayName: 'Chrome',
+    versionRegex: /Google Chrome (\S+)/,
+    profile: true,
+    binary: 'google-chrome'
+  },{
+    name: 'chromium',
+    displayName: 'Chromium',
+    versionRegex: /Chromium (\S+)/,
+    profile: true,
+    binary: 'chromium-browser'
+  },{
+    name: 'canary',
+    displayName: 'Canary',
+    versionRegex: /Google Chrome Canary (\S+)/,
+    profile: true,
+    binary: 'google-chrome-canary'
+  },
+  // a couple of fallbacks
+  googleChromeStable,
+  googleChromeAlias
+]
+
 /** starts a browser by name and opens URL if given one */
 export function launch (browsers: FoundBrowser[],
   name: string, url?: string, args: string[] = []) {
diff --git a/packages/launcher/lib/darwin/canary.ts b/packages/launcher/lib/darwin/canary.ts
deleted file mode 100644
index 79957f2ae6..0000000000
--- a/packages/launcher/lib/darwin/canary.ts
+++ /dev/null
@@ -1,22 +0,0 @@
-import {parse, find} from './util'
-import path = require('path')
-import Promise = require('bluebird')
-
-const canary = {
-  version: (p: string) =>
-    parse(p, 'KSVersion'),
-
-  path: () => find('com.google.Chrome.canary'),
-
-  get (executable: string) {
-    return this.path()
-      .then (p => {
-        return Promise.props({
-          path:    path.join(p, executable),
-          version: this.version(p)
-        })
-      })
-  }
-}
-
-export default canary
diff --git a/packages/launcher/lib/darwin/chrome.ts b/packages/launcher/lib/darwin/chrome.ts
deleted file mode 100644
index 5a37a8ee05..0000000000
--- a/packages/launcher/lib/darwin/chrome.ts
+++ /dev/null
@@ -1,28 +0,0 @@
-import {log} from '../log'
-
-import {parse, find} from './util'
-import path = require('path')
-import Promise = require('bluebird')
-
-const chrome = {
-  version (p: string) {
-    return parse(p, 'KSVersion')
-  },
-
-  path () {
-    return find('com.google.Chrome')
-  },
-
-  get (executable: string) {
-    log('Looking for Chrome %s', executable)
-    return this.path()
-      .then(p => {
-        return Promise.props({
-          path:    path.join(p, executable),
-          version: this.version(p)
-        })
-      })
-  }
-}
-
-export default chrome
diff --git a/packages/launcher/lib/darwin/chromium.ts b/packages/launcher/lib/darwin/chromium.ts
deleted file mode 100644
index 8fde0501f4..0000000000
--- a/packages/launcher/lib/darwin/chromium.ts
+++ /dev/null
@@ -1,25 +0,0 @@
-import {find, parse} from './util'
-import path = require('path')
-import Promise = require('bluebird')
-
-const chromium = {
-  version (p: string) {
-    return parse(p, 'CFBundleShortVersionString')
-  },
-
-  path () {
-    return find('org.chromium.Chromium')
-  },
-
-  get (executable: string) {
-    return this.path()
-    .then(p =>
-      Promise.props({
-        path:    path.join(p, executable),
-        version: this.version(p)
-      })
-    )
-  }
-}
-
-export default chromium
diff --git a/packages/launcher/lib/darwin/index.ts b/packages/launcher/lib/darwin/index.ts
index 52f3a7e322..ea183db996 100644
--- a/packages/launcher/lib/darwin/index.ts
+++ b/packages/launcher/lib/darwin/index.ts
@@ -1,11 +1,40 @@
-import canary from './canary'
-import chrome from './chrome'
-import chromium from './chromium'
+import {findApp} from './util'
+import {Browser} from '../types'
+import {detectBrowserLinux} from '../linux'
+import {log} from '../log'
+import {merge, partial} from 'ramda'
 
-const browsers = {
-  chrome,
-  canary,
-  chromium
+const detectCanary = partial(findApp,
+  ['Contents/MacOS/Google Chrome Canary', 'com.google.Chrome.canary', 'KSVersion'])
+const detectChrome = partial(findApp,
+  ['Contents/MacOS/Google Chrome', 'com.google.Chrome', 'KSVersion'])
+const detectChromium = partial(findApp,
+  ['Contents/MacOS/Chromium', 'org.chromium.Chromium', 'CFBundleShortVersionString'])
+
+type Detectors = {
+  [index: string]: Function
 }
 
-export default browsers
+const browsers: Detectors = {
+  chrome: detectChrome,
+  canary: detectCanary,
+  chromium: detectChromium
+}
+
+export function detectBrowserDarwin (browser: Browser) {
+  let fn = browsers[browser.name]
+
+  if (!fn) {
+    // ok, maybe it is custom alias?
+    log('detecting custom browser %s on darwin', browser.name)
+    return detectBrowserLinux(browser)
+  }
+
+  return fn()
+    .then(merge({name: browser.name}))
+    .catch(() => {
+      log('could not detect %s using traditional Mac methods', browser.name)
+      log('trying linux search')
+      return detectBrowserLinux(browser)
+    })
+}
diff --git a/packages/launcher/lib/darwin/util.ts b/packages/launcher/lib/darwin/util.ts
index 71af9ac376..c247f15cc1 100644
--- a/packages/launcher/lib/darwin/util.ts
+++ b/packages/launcher/lib/darwin/util.ts
@@ -7,7 +7,8 @@ import fs = require('fs-extra')
 import path = require('path')
 import plist = require('plist')
 
-export function parse (p: string, property: string) {
+/** parses Info.plist file from given application and returns a property */
+export function parse (p: string, property: string): Promise<string> {
   const pl = path.join(p, 'Contents', 'Info.plist')
   log('reading property file "%s"', pl)
 
@@ -26,7 +27,8 @@ export function parse (p: string, property: string) {
     .catch(failed)
 }
 
-export function find (id: string): Promise<string> {
+/** uses mdfind to find app using Ma app id like 'com.google.Chrome.canary' */
+export function mdfind (id: string): Promise<string> {
   const cmd = `mdfind 'kMDItemCFBundleIdentifier=="${id}"' | head -1`
   log('looking for bundle id %s using command: %s', id, cmd)
 
@@ -47,3 +49,43 @@ export function find (id: string): Promise<string> {
     .then(tap(logFound))
     .catch(failedToFind)
 }
+
+export type AppInfo = {
+  path: string,
+  version: string
+}
+
+function formApplicationPath (executable: string) {
+  const parts = executable.split('/')
+  const name = parts[parts.length - 1]
+  const appName = `${name}.app`
+  return path.join('/Applications', appName)
+}
+
+/** finds an application and its version */
+export function findApp (executable: string, appId: string, versionProperty: string): Promise<AppInfo> {
+  log('looking for app %s id %s', executable, appId)
+
+  const findVersion = (foundPath: string) =>
+    parse(foundPath, versionProperty)
+      .then((version) => {
+        return {
+          path: path.join(foundPath, executable),
+          version
+        }
+      })
+
+  const tryMdFind = () => {
+    return mdfind(appId)
+      .then(findVersion)
+  }
+
+  const tryFullApplicationFind = () => {
+    const applicationPath = formApplicationPath(executable)
+    log('looking for application %s', applicationPath)
+    return findVersion(applicationPath)
+  }
+
+  return tryMdFind()
+    .catch(tryFullApplicationFind)
+}
diff --git a/packages/launcher/lib/detect.ts b/packages/launcher/lib/detect.ts
index 5770b9be8c..851e0fa97a 100644
--- a/packages/launcher/lib/detect.ts
+++ b/packages/launcher/lib/detect.ts
@@ -1,35 +1,13 @@
-import {linuxBrowser} from './linux'
-import darwin from './darwin'
+import {detectBrowserLinux} from './linux'
+import {detectBrowserDarwin} from './darwin'
 import {log} from './log'
 import {Browser, NotInstalledError} from './types'
+import {browsers} from './browsers'
 import * as Bluebird from 'bluebird'
-import {merge, pick, tap} from 'ramda'
+import {merge, pick, tap, uniqBy, prop} from 'ramda'
 
 import _ = require('lodash')
 import os = require('os')
-// import Promise = require('bluebird')
-
-const browsers: Browser[] = [
-  {
-    name: 'chrome',
-    re: /Google Chrome (\S+)/,
-    profile: true,
-    binary: 'google-chrome',
-    executable: 'Contents/MacOS/Google Chrome'
-  },{
-    name: 'chromium',
-    re: /Chromium (\S+)/,
-    profile: true,
-    binary: 'chromium-browser',
-    executable: 'Contents/MacOS/Chromium'
-  },{
-    name: 'canary',
-    re: /Google Chrome Canary (\S+)/,
-    profile: true,
-    binary: 'google-chrome-canary',
-    executable: 'Contents/MacOS/Google Chrome Canary'
-  }
-]
 
 const setMajorVersion = (obj: Browser) => {
   if (obj.version) {
@@ -40,33 +18,29 @@ const setMajorVersion = (obj: Browser) => {
   return obj
 }
 
-type MacBrowserName = 'chrome' | 'chromium' | 'canary'
+type BrowserDetector = (browser: Browser) => Promise<Object>
+type Detectors = {
+  [index: string]: BrowserDetector
+}
+const detectors: Detectors = {
+  darwin: detectBrowserDarwin,
+  linux: detectBrowserLinux
+}
 
-function lookup (platform: string, obj: Browser): Promise<Object> {
+function lookup (platform: NodeJS.Platform, obj: Browser): Promise<Object> {
   log('looking up %s on %s platform', obj.name, platform)
-  switch (platform) {
-    case 'darwin':
-      const browserName: MacBrowserName = obj.name as MacBrowserName
-      const fn = darwin[browserName]
-      if (fn) {
-        return fn.get(obj.executable) as any as Promise<Object>
-      }
-      const err: NotInstalledError =
-        new Error(`Browser not installed: ${obj.name}`) as NotInstalledError
-      err.notInstalled = true
-      throw err
-    case 'linux':
-      return linuxBrowser.get(obj.binary, obj.re) as any as Promise<Object>
-    default:
-      throw new Error(`Cannot lookup browser ${obj.name} on ${platform}`)
+  const detector = detectors[platform]
+  if (!detector) {
+    throw new Error(`Cannot lookup browser ${obj.name} on ${platform}`)
   }
+  return detector(obj)
 }
 
 function checkOneBrowser (browser: Browser) {
   const platform = os.platform()
-  const pickBrowserProps = pick(['name', 'type', 'version', 'path'])
+  const pickBrowserProps = pick(['name', 'displayName', 'type', 'version', 'path'])
 
-  const logBrowser = (props: object) => {
+  const logBrowser = (props: any) => {
     log('setting major version for %j', props)
   }
 
@@ -88,8 +62,12 @@ function checkOneBrowser (browser: Browser) {
 
 /** returns list of detected browsers */
 function detectBrowsers (): Bluebird<Browser[]> {
+  // we can detect same browser under different aliases
+  // tell them apart by the full version property
+  const removeDuplicates = uniqBy(prop('version'))
   return Bluebird.mapSeries(browsers, checkOneBrowser)
-    .then(_.compact) as Bluebird<Browser[]>
+    .then(_.compact)
+    .then(removeDuplicates) as Bluebird<Browser[]>
 }
 
 export default detectBrowsers
diff --git a/packages/launcher/lib/linux/index.ts b/packages/launcher/lib/linux/index.ts
index a435b920ea..793952cbc0 100644
--- a/packages/launcher/lib/linux/index.ts
+++ b/packages/launcher/lib/linux/index.ts
@@ -1,30 +1,40 @@
-import cp = require('child_process')
-import Promise = require('bluebird')
-import {NotInstalledError} from '../types'
-
-const execAsync = Promise.promisify(cp.exec)
+import {log} from '../log'
+import {prop, trim} from 'ramda'
+import {FoundBrowser, Browser, NotInstalledError} from '../types'
+import execa = require('execa')
 
 const notInstalledErr = (name: string) => {
-  const err: NotInstalledError = new Error(`Browser not installed: ${name}`) as NotInstalledError
+  const err: NotInstalledError =
+    new Error(`Browser not installed: ${name}`) as NotInstalledError
   err.notInstalled = true
   throw err
 }
 
-export const linuxBrowser = {
-  get: (binary: string, re: RegExp): Promise<any> => {
-    return execAsync(`${binary} --version`)
-      .call('trim')
-      .then (stdout => {
-        const m = re.exec(stdout)
-        if (m) {
-          return {
-            path: binary,
-            version: m[1]
-          }
-        } else {
-          return notInstalledErr(binary)
-        }
-      })
-      .catch(() => notInstalledErr(binary))
+function getLinuxBrowser (name: string, binary: string, versionRegex: RegExp): Promise<FoundBrowser> {
+  const getVersion = (stdout: string) => {
+    const m = versionRegex.exec(stdout)
+    if (m) {
+      return m[1]
+    }
+    return notInstalledErr(binary)
   }
+
+  const cmd = `${binary} --version`
+  log('looking using command "%s"', cmd)
+  return execa.shell(cmd)
+    .then(prop('stdout'))
+    .then(trim)
+    .then(getVersion)
+    .then((version) => {
+      return {
+        name,
+        version,
+        path: binary
+      }
+    })
+    .catch(() => notInstalledErr(binary))
+}
+
+export function detectBrowserLinux (browser: Browser) {
+  return getLinuxBrowser(browser.name, browser.binary, browser.versionRegex)
 }
diff --git a/packages/launcher/lib/types.ts b/packages/launcher/lib/types.ts
index 09968fc510..47e28e2bf6 100644
--- a/packages/launcher/lib/types.ts
+++ b/packages/launcher/lib/types.ts
@@ -1,14 +1,27 @@
+/** TODO this are typical browser names, not just Mac */
+export type MacBrowserName = 'chrome' | 'chromium' | 'canary' | string
+
+export type PlatformName = 'darwin' | 'linux'
+
 export type Browser = {
-  name: string,
-  re: RegExp,
+  /** short browser name */
+  name: MacBrowserName,
+  /** Optional display name */
+  displayName?: string,
+  /** RegExp to use to extract version from something like "Google Chrome 58.0.3029.110" */
+  versionRegex: RegExp,
   profile: boolean,
   binary: string,
-  executable: string,
   version?: string,
   majorVersion?: string,
   page?: string
 }
 
+export type FoundBrowser = {
+  name: string,
+  path?: string
+}
+
 interface ExtraLauncherMethods {
   update: Function,
   detect: Function
diff --git a/packages/launcher/package.json b/packages/launcher/package.json
index 15fdf16bec..4d9f5a9899 100644
--- a/packages/launcher/package.json
+++ b/packages/launcher/package.json
@@ -32,7 +32,7 @@
     "mocha": "^2.4.5",
     "sinon": "^1.17.3",
     "sinon-chai": "^2.8.0",
-    "tslint": "5.2.0",
+    "tslint": "5.3.2",
     "tslint-config-standard": "5.0.2",
     "typescript": "2.3.2"
   },