Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Modify doubleEscape to handle path with extra backslashes and launch edge without browser key #14723

Merged
merged 8 commits into from
Feb 8, 2021
30 changes: 20 additions & 10 deletions packages/launcher/lib/windows/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import fse from 'fs-extra'
import os from 'os'
import { join, normalize } from 'path'
import { join, normalize, win32 } from 'path'
import { tap, trim, prop } from 'ramda'
import { get } from 'lodash'
import { notInstalledErr } from '../errors'
Expand All @@ -21,6 +21,12 @@ function formChromiumAppPath () {
return [normalize(exe)]
}

function doubleEscape (s: string) {
// Converts all types of paths into windows supported double backslash path
// Handles any number of \\ in the given path
return win32.join(...s.split(win32.sep)).replace(/\\/g, '\\\\')
}

function formChromeCanaryAppPath () {
const home = os.homedir()
const exe = join(
Expand Down Expand Up @@ -132,15 +138,17 @@ function getWindowsBrowser (browser: Browser): Promise<FoundBrowser> {
throw notInstalledErr(browser.name)
}

return fse.pathExists(exePath)
let path = doubleEscape(exePath)

return fse.pathExists(path)
.then((exists) => {
log('found %s ?', exePath, exists)
log('found %s ?', path, exists)

if (!exists) {
return tryNextExePath()
}

return getVersionString(exePath)
return getVersionString(path)
.then(tap(log))
.then(getVersion)
.then((version: string) => {
Expand All @@ -164,17 +172,13 @@ function getWindowsBrowser (browser: Browser): Promise<FoundBrowser> {
}

export function getVersionString (path: string) {
const doubleEscape = (s: string) => {
return s.replace(/\\/g, '\\\\')
}

// on Windows using "--version" seems to always start the full
// browser, no matter what one does.

const args = [
'datafile',
'where',
`name="${doubleEscape(path)}"`,
`name="${path}"`,
'get',
'Version',
'/value',
Expand Down Expand Up @@ -203,15 +207,21 @@ export function getPathData (pathStr: string): PathData {
const pathParts = path.split(':')

browserKey = pathParts.pop() || ''
path = pathParts.join(':')
path = doubleEscape(pathParts.join(':'))

return { path, browserKey }
}

path = doubleEscape(path)

if (pathStr.indexOf('chrome.exe') > -1) {
return { path, browserKey: 'chrome' }
}

if (pathStr.indexOf('edge.exe') > -1) {
return { path, browserKey: 'edge' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, I guess this does technically work for msedge.exe too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, keeping that browserKey edge for msedge.exe too as we use edge everywhere else.

}

if (pathStr.indexOf('firefox.exe') > -1) {
return { path, browserKey: 'firefox' }
}
Expand Down
93 changes: 77 additions & 16 deletions packages/launcher/test/unit/windows_spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import _ from 'lodash'
import { expect } from 'chai'
import * as windowsHelper from '../../lib/windows'
import { normalize } from 'path'
import { normalize, win32 } from 'path'
import { utils } from '../../lib/utils'
import sinon, { SinonStub } from 'sinon'
import { browsers } from '../../lib/browsers'
Expand All @@ -14,7 +14,7 @@ import { detectByPath } from '../../lib/detect'
import { goalBrowsers } from '../fixtures'

function stubBrowser (path: string, version: string) {
path = normalize(path.replace(/\\/g, '\\\\'))
path = doubleEscape(normalize(path))

;(utils.execa as unknown as SinonStub)
.withArgs('wmic', ['datafile', 'where', `name="${path}"`, 'get', 'Version', '/value'])
Expand All @@ -25,6 +25,10 @@ function stubBrowser (path: string, version: string) {
.resolves(true)
}

function doubleEscape (s: string) {
return win32.join(...s.split(win32.sep)).replace(/\\/g, '\\\\')
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you just import this from windows.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean packages/launcher/lib/windows/index.ts

Exported that function, used it in the test case and added its own test cases

function detect (goalBrowsers: Browser[]) {
return Bluebird.mapSeries(goalBrowsers, (browser) => {
return windowsHelper.detect(browser)
Expand Down Expand Up @@ -91,39 +95,45 @@ describe('windows browser detection', () => {

it('works with :browserName format in Windows', () => {
sinon.stub(os, 'platform').returns('win32')
stubBrowser(`${HOMEDIR}/foo/bar/browser.exe`, '100')
let path = `${HOMEDIR}/foo/bar/browser.exe`
let win10Path = doubleEscape(path)

return detectByPath(`${HOMEDIR}/foo/bar/browser.exe:foo-browser`, goalBrowsers as Browser[]).then((browser) => {
stubBrowser(path, '100')

return detectByPath(`${path}:foo-browser`, goalBrowsers as Browser[]).then((browser) => {
expect(browser).to.deep.equal(
Object.assign({}, goalBrowsers.find((gb) => {
return gb.name === 'foo-browser'
}), {
displayName: 'Custom Foo Browser',
info: `Loaded from ${HOMEDIR}/foo/bar/browser.exe`,
info: `Loaded from ${win10Path}`,
custom: true,
version: '100',
majorVersion: 100,
path: `${HOMEDIR}/foo/bar/browser.exe`,
path: win10Path,
}),
)
})
})

it('identifies browser if name in path', async () => {
sinon.stub(os, 'platform').returns('win32')
stubBrowser(`${HOMEDIR}/foo/bar/chrome.exe`, '100')
let path = `${HOMEDIR}/foo/bar/chrome.exe`
let win10Path = doubleEscape(path)

stubBrowser(path, '100')

return detectByPath(`${HOMEDIR}/foo/bar/chrome.exe`).then((browser) => {
return detectByPath(path).then((browser) => {
expect(browser).to.deep.equal(
Object.assign({}, browsers.find((gb) => {
return gb.name === 'chrome'
}), {
displayName: 'Custom Chrome',
info: `Loaded from ${HOMEDIR}/foo/bar/chrome.exe`,
info: `Loaded from ${win10Path}`,
custom: true,
version: '100',
majorVersion: 100,
path: `${HOMEDIR}/foo/bar/chrome.exe`,
path: win10Path,
}),
)
})
Expand All @@ -149,23 +159,74 @@ describe('windows browser detection', () => {

context('#getPathData', () => {
it('returns path and browserKey given path with browser key', () => {
const res = windowsHelper.getPathData('C:\\foo\\bar.exe:firefox')
const browserPath = 'C:\\foo\\bar.exe'
const res = windowsHelper.getPathData(`${browserPath}:firefox`)

expect(res.path).to.eq('C:\\foo\\bar.exe')
expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('firefox')
})

it('returns path and browserKey given path with a lot of slashes plus browser key', () => {
const browserPath = 'C:\\\\\\\\foo\\\\\\bar.exe'
const res = windowsHelper.getPathData(`${browserPath}:firefox`)

expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('firefox')
})

it('returns path and browserKey given nix path with browser key', () => {
const browserPath = 'C:/foo/bar.exe'
const res = windowsHelper.getPathData(`${browserPath}:firefox`)

expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('firefox')
})

it('returns path and chrome given just path', () => {
const res = windowsHelper.getPathData('C:\\foo\\bar\\chrome.exe')
const browserPath = 'C:\\foo\\bar\\chrome.exe'
const res = windowsHelper.getPathData(browserPath)

expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('chrome')
})

expect(res.path).to.eq('C:\\foo\\bar\\chrome.exe')
it('returns path and chrome given just nix path', () => {
const browserPath = 'C:/foo/bar/chrome.exe'
const res = windowsHelper.getPathData(browserPath)

expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('chrome')
})

it('returns path and edge given just path', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also use a test for msedge.exe - that is what #14716 is about

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, updated

const browserPath = 'C:\\foo\\bar\\edge.exe'
const res = windowsHelper.getPathData(browserPath)

expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('edge')
})

it('returns path and edge given just nix path', () => {
const browserPath = 'C:/foo/bar/edge.exe'
const res = windowsHelper.getPathData(browserPath)

expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('edge')
})

it('returns path and firefox given just path', () => {
const res = windowsHelper.getPathData('C:\\foo\\bar\\firefox.exe')
const browserPath = 'C:\\foo\\bar\\firefox.exe'
const res = windowsHelper.getPathData(browserPath)

expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('firefox')
})

it('returns path and firefox given just nix path', () => {
const browserPath = 'C:/foo/bar/firefox.exe'
const res = windowsHelper.getPathData(browserPath)

expect(res.path).to.eq('C:\\foo\\bar\\firefox.exe')
expect(res.path).to.eq(doubleEscape(browserPath))
expect(res.browserKey).to.eq('firefox')
})
})
Expand Down