Skip to content

Commit

Permalink
Fix: PR Feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@molant
molant committed Apr 18, 2019
1 parent 6a5bdb4 commit a8d4c92
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 38 deletions.
17 changes: 10 additions & 7 deletions packages/utils/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,16 @@ and exception.

### chromiumFinder

* `Browser`: An enum with the Chromium based browsers supported: Chrome, Chromium, and Edge.
* `getInstallationPath`: Searchs for a valid Chromium browser from the ones supported. The current priority list is:
* `Chrome Canary`, `Chrome`, `Chromium`, `Edge Canary`, `Edge Dev` (`Edge` only on `win32 platforms).
A user can also pass the browser to use (`Chrome`, `Chromium`, `Edge`) via the `options`
parameter (`options.browser`) or a `path` to the executable (`options.browserPath`) to
use (`getInstallationPath` will only verify it exists, not if it's actually a valid
target).
* `Browser`: An enum with the Chromium based browsers supported: Chrome,
Chromium, and Edge.
* `getInstallationPath`: Searchs for a valid Chromium browser from the ones
supported. The current priority list is:
* `Chrome Canary`, `Chrome`, `Chromium`, `Edge Canary`, `Edge Dev` (`Edge`
only on `win32` platforms). A user can also pass the browser to use
(`Chrome`, `Chromium`, `Edge`) via the `options` parameter
(`options.browser`) or a `path` to the executable (`options.browserPath`) to
use (`getInstallationPath` will only verify it exists, not if it's actually
a valid target).

### dom

Expand Down
44 changes: 21 additions & 23 deletions packages/utils/src/chromium-finder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
*/

import * as path from 'path';
import { isFile as canAccess } from './fs/is-file';
import { isFile } from './fs/is-file';
import { isDirectory } from './fs/is-directory';
import { getVariable, setVariable } from './misc/environment';
import { getPlatform } from './misc/get-platform';

Expand All @@ -20,6 +21,7 @@ export enum Browser {
}

const ERRORS = {
InvalidPath: 'The provided path is not accessible',
NoInstallationFound: 'No installation found',
NotSupportedBrowser: 'The provided browser is not supported in this platform',
UnsupportedPlatform: 'Unsupported platform'
Expand Down Expand Up @@ -92,8 +94,8 @@ const browserVariables = new Map([

/** Find chromium executables on macOS */
const darwin = (browser: Browser) => {
const platformBroserInfo = browserVariables.get('darwin');
const suffixes = platformBroserInfo!.get(browser);
const platformBrowserInfo = browserVariables.get('darwin')!;
const suffixes = platformBrowserInfo.get(browser);

if (!suffixes) {
throw new Error(ERRORS.NotSupportedBrowser);
Expand Down Expand Up @@ -133,7 +135,7 @@ const darwin = (browser: Browser) => {
* An example of valid path for Chrome would be:
* `/Applications/Google Chrome.app/Contents/MacOS/Google Chrome`
*/
if (canAccess(execPath)) {
if (isFile(execPath)) {
return execPath;
}
}
Expand All @@ -150,7 +152,7 @@ const findChromeExecutable = (folder: string, regExes: string[]) => {

const chromeExecRegex = `^Exec=/.*/${browserRegex}-.*`;

if (canAccess(folder)) {
if (isDirectory(folder)) {
/*
* Output of the grep & print looks like:
* /opt/google/chrome/google-chrome --profile-directory
Expand All @@ -175,7 +177,7 @@ const findChromeExecutable = (folder: string, regExes: string[]) => {
});

for (const execPath of execPaths) {
if (canAccess(execPath)) {
if (isFile(execPath)) {
return execPath;
}
}
Expand All @@ -192,14 +194,14 @@ const findChromeExecutable = (folder: string, regExes: string[]) => {
*/
const linux = (browser: Browser) => {

// 2. Look into the directories where .desktop are saved on gnome based distro's
// 1. Look into the directories where .desktop are saved on gnome based distro's
const desktopInstallationFolders = [
path.join(require('os').homedir(), '.local/share/applications/'),
'/usr/share/applications/'
];

const browserPartsInfo = browserVariables.get('linux');
const executables = browserPartsInfo!.get(browser);
const browserPartsInfo = browserVariables.get('linux')!;
const executables = browserPartsInfo.get(browser);

if (!executables) {
throw new Error(ERRORS.NotSupportedBrowser);
Expand All @@ -213,14 +215,14 @@ const linux = (browser: Browser) => {
}
}

// Look for executables by using the which command
// 2. Look for executables by using the which command
for (const executable of executables) {
try {
const chromePath =
execFileSync('which', [executable], { stdio: 'pipe' }).toString()
.split(newLineRegex)[0];

if (chromePath && canAccess(chromePath)) {
if (chromePath && isFile(chromePath)) {
return chromePath;
}
} catch (e) {
Expand All @@ -236,11 +238,7 @@ const linux = (browser: Browser) => {
* @param browser The type of browser to search for
*/
const win32 = (browser: Browser) => {
const info = browserVariables.get('win32');

if (!info) {
throw new Error(ERRORS.NotSupportedBrowser);
}
const info = browserVariables.get('win32')!;

const suffixes = info.get(browser);

Expand All @@ -258,7 +256,7 @@ const win32 = (browser: Browser) => {
for (const prefix of prefixes) {
const browserPath = path.join(prefix!, suffix);

if (canAccess(browserPath)) {
if (isFile(browserPath)) {
return browserPath;
}
}
Expand Down Expand Up @@ -318,9 +316,9 @@ const resolveChromiumPath = () => {
];

while (chromiumPaths.length > 0) {
const browserPath = chromiumPaths.shift();
const browserPath = chromiumPaths.shift()!;

if (canAccess(browserPath!)) {
if (isFile(browserPath)) {
return browserPath;
}
}
Expand Down Expand Up @@ -350,12 +348,12 @@ const resolveChromiumPath = () => {
*/
export const getInstallationPath = (options?: { browser?: Browser; browserPath?: string }) => {
if (options && options.browserPath) {
if (canAccess(options.browserPath)) {
if (isFile(options.browserPath)) {
return options.browserPath;
}

// The provided path is not accessible
throw new Error(`The path "${options.browserPath}" is not accessible`);
throw new Error(ERRORS.InvalidPath);
}

const resolvedChromiumPath = resolveChromiumPath();
Expand All @@ -373,10 +371,10 @@ export const getInstallationPath = (options?: { browser?: Browser; browserPath?:
let browserFound = '';

while (browsers.length > 0 && !browserFound) {
const br = browsers.shift();
const br = browsers.shift()!;

try {
browserFound = findBrowserPath(br!);
browserFound = findBrowserPath(br);
} catch (e) {
// keep searching
}
Expand Down
6 changes: 3 additions & 3 deletions packages/utils/src/fs/is-file.ts
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { accessSync } from 'fs';
import { statSync } from 'fs';

/** Check if a path is a file and exists. */
export const isFile = (filePath?: string): boolean => {
try {
accessSync(filePath!);
const stats = statSync(filePath!);

return true;
return stats.isFile();
} catch (e) {
return false;
}
Expand Down
12 changes: 7 additions & 5 deletions packages/utils/tests/chromium-finder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,11 @@ test(`Invalid custom path throws an exception`, (t) => {

const chromiumFinder = loadDependency(t.context);

t.throws(() => {
const error = t.throws(() => {
chromiumFinder.getInstallationPath({ browserPath: 'invalid' });
});

t.is(error.message, 'The provided path is not accessible', `Error message is not the expected one`);
});

test(`Searches with the right priorities and throws an exception when nothing is found`, (t) => {
Expand Down Expand Up @@ -182,10 +184,10 @@ test(`wsl sets missing env variables Windows`, (t) => {
chromiumFinder.getInstallationPath({ browser: 'Chrome' });
});

t.true(setVariableSpy.calledThrice, `setVariable isn't called 3 times`);
t.is(setVariableSpy.firstCall.args[0], 'LOCALAPPDATA', `First 'setVariable' call isn't LOCALAPPDATA`);
t.is(setVariableSpy.secondCall.args[0], 'PROGRAMFILES', `Second 'setVariable' call isn't PROGRAMFILES`);
t.is(setVariableSpy.thirdCall.args[0], 'PROGRAMFILES(X86)', `Third 'setVariable' call isn't PROGRAMFILES(X86)`);
t.true(setVariableSpy.calledThrice);
t.is(setVariableSpy.firstCall.args[0], 'LOCALAPPDATA');
t.is(setVariableSpy.secondCall.args[0], 'PROGRAMFILES');
t.is(setVariableSpy.thirdCall.args[0], 'PROGRAMFILES(X86)');
});

test(`(macOS) Does not have any information for Edge`, (t) => {
Expand Down

0 comments on commit a8d4c92

Please sign in to comment.