Skip to content

Commit

Permalink
refactor package manager proxies and use execa instead of cross-spawn
Browse files Browse the repository at this point in the history
  • Loading branch information
@yannbf
yannbf committed May 4, 2023
1 parent 7e57324 commit ef4cc90
Show file tree
Hide file tree
Showing 9 changed files with 328 additions and 235 deletions.
80 changes: 55 additions & 25 deletions code/lib/cli/src/js-package-manager/JsPackageManager.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import chalk from 'chalk';
import { gt, satisfies } from 'semver';
import { sync as spawnSync } from 'cross-spawn';
import type { CommonOptions } from 'execa';
import { sync as commandSync } from 'execa';
import path from 'path';
import fs from 'fs';

import dedent from 'ts-dedent';
import { commandLog } from '../helpers';
import type { PackageJson, PackageJsonWithDepsAndDevDeps } from './PackageJson';
import storybookPackagesVersions from '../versions';
Expand Down Expand Up @@ -44,23 +46,23 @@ export abstract class JsPackageManager {

public abstract getRunCommand(command: string): string;

public readonly cwd?: string;

// NOTE: for some reason yarn prefers the npm registry in
// local development, so always use npm
setRegistryURL(url: string) {
if (url) {
this.executeCommand('npm', ['config', 'set', 'registry', url]);
this.executeCommand({ command: 'npm', args: ['config', 'set', 'registry', url] });
} else {
this.executeCommand('npm', ['config', 'delete', 'registry']);
this.executeCommand({ command: 'npm', args: ['config', 'delete', 'registry'] });
}
}

getRegistryURL() {
const url = this.executeCommand('npm', ['config', 'get', 'registry']).trim();
const url = this.executeCommand({ command: 'npm', args: ['config', 'get', 'registry'] }).trim();
return url === 'undefined' ? undefined : url;
}

public readonly cwd?: string;

constructor(options?: JsPackageManagerOptions) {
this.cwd = options?.cwd;
}
Expand Down Expand Up @@ -134,8 +136,19 @@ export abstract class JsPackageManager {
try {
packageJson = this.readPackageJson();
} catch (err) {
this.initPackageJson();
packageJson = this.readPackageJson();
if (err.message.includes('Could not read package.json')) {
this.initPackageJson();
packageJson = this.readPackageJson();
} else {
throw new Error(
dedent`
There was an error while reading the package.json file at ${this.packageJsonPath()}: ${
err.message
}
Please fix the error and try again.
`
);
}
}

return {
Expand Down Expand Up @@ -297,6 +310,11 @@ export abstract class JsPackageManager {
if (/(@storybook|^sb$|^storybook$)/.test(packageName)) {
// @ts-expect-error (Converted from ts-ignore)
current = storybookPackagesVersions[packageName];

// HEY THERE! IF THIS IS COMMITED IT WAS A MISTAKE. PLEASE UNDO THIS:
if (current) {
return `^${current}`;
}
}

let latest;
Expand Down Expand Up @@ -412,24 +430,36 @@ export abstract class JsPackageManager {
public abstract runPackageCommand(command: string, args: string[], cwd?: string): string;
public abstract findInstallations(pattern?: string[]): InstallationMetadata | undefined;

public executeCommand(
command: string,
args: string[],
stdio?: 'pipe' | 'inherit',
cwd?: string,
ignoreError?: boolean
): string {
const commandResult = spawnSync(command, args, {
cwd: cwd ?? this.cwd,
stdio: stdio ?? 'pipe',
encoding: 'utf-8',
shell: true,
});
public executeCommand({
command,
args = [],
stdio,
cwd,
ignoreError = false,
env,
}: {
command: string;
args: string[];
stdio?: CommonOptions<string>['stdio'];
cwd?: string;
ignoreError?: boolean;
env?: CommonOptions<string>['env'];
}): string {
try {
const commandResult = commandSync(command, args, {
cwd: cwd ?? this.cwd,
stdio: stdio ?? 'pipe',
encoding: 'utf-8',
shell: true,
env,
});

if (commandResult.status !== 0 && ignoreError !== true) {
throw new Error(commandResult.stderr ?? '');
return commandResult.stdout ?? '';
} catch (err) {
if (ignoreError !== true) {
throw err;
}
return '';
}

return commandResult.stdout ?? '';
}
}
102 changes: 51 additions & 51 deletions code/lib/cli/src/js-package-manager/NPMProxy.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ describe('NPM Proxy', () => {

npmProxy.initPackageJson();

expect(executeCommandSpy).toHaveBeenCalledWith('npm', ['init', '-y']);
expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({ command: 'npm', args: ['init', '-y'] })
);
});
});

Expand All @@ -27,12 +29,12 @@ describe('NPM Proxy', () => {

npmProxy.setRegistryURL('https://foo.bar');

expect(executeCommandSpy).toHaveBeenCalledWith('npm', [
'config',
'set',
'registry',
'https://foo.bar',
]);
expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['config', 'set', 'registry', 'https://foo.bar'],
})
);
});
});

Expand Down Expand Up @@ -65,10 +67,10 @@ describe('NPM Proxy', () => {
npmProxy.runPackageCommand('compodoc', ['-e', 'json', '-d', '.']);

expect(executeCommandSpy).toHaveBeenLastCalledWith(
'npm',
['exec', '--', 'compodoc', '-e', 'json', '-d', '.'],
undefined,
undefined
expect.objectContaining({
command: 'npm',
args: ['exec', '--', 'compodoc', '-e', 'json', '-d', '.'],
})
);
});
});
Expand All @@ -79,10 +81,10 @@ describe('NPM Proxy', () => {
npmProxy.runPackageCommand('compodoc', ['-e', 'json', '-d', '.']);

expect(executeCommandSpy).toHaveBeenLastCalledWith(
'npm',
['exec', '--', 'compodoc', '-e', 'json', '-d', '.'],
undefined,
undefined
expect.objectContaining({
command: 'npm',
args: ['exec', '--', 'compodoc', '-e', 'json', '-d', '.'],
})
);
});
});
Expand All @@ -96,9 +98,10 @@ describe('NPM Proxy', () => {
npmProxy.addDependencies({ installAsDevDependencies: true }, ['@storybook/preview-api']);

expect(executeCommandSpy).toHaveBeenLastCalledWith(
'npm',
['install', '-D', '@storybook/preview-api'],
expect.any(String)
expect.objectContaining({
command: 'npm',
args: ['install', '-D', '@storybook/preview-api'],
})
);
});
});
Expand All @@ -109,9 +112,10 @@ describe('NPM Proxy', () => {
npmProxy.addDependencies({ installAsDevDependencies: true }, ['@storybook/preview-api']);

expect(executeCommandSpy).toHaveBeenLastCalledWith(
'npm',
['install', '-D', '@storybook/preview-api'],
expect.any(String)
expect.objectContaining({
command: 'npm',
args: ['install', '-D', '@storybook/preview-api'],
})
);
});
});
Expand All @@ -125,9 +129,7 @@ describe('NPM Proxy', () => {
npmProxy.removeDependencies({}, ['@storybook/preview-api']);

expect(executeCommandSpy).toHaveBeenLastCalledWith(
'npm',
['uninstall', '@storybook/preview-api'],
expect.any(String)
expect.objectContaining({ command: 'npm', args: ['uninstall', '@storybook/preview-api'] })
);
});
});
Expand All @@ -138,9 +140,7 @@ describe('NPM Proxy', () => {
npmProxy.removeDependencies({}, ['@storybook/preview-api']);

expect(executeCommandSpy).toHaveBeenLastCalledWith(
'npm',
['uninstall', '@storybook/preview-api'],
expect.any(String)
expect.objectContaining({ command: 'npm', args: ['uninstall', '@storybook/preview-api'] })
);
});
});
Expand Down Expand Up @@ -180,12 +180,12 @@ describe('NPM Proxy', () => {

const version = await npmProxy.latestVersion('@storybook/preview-api');

expect(executeCommandSpy).toHaveBeenCalledWith('npm', [
'info',
'@storybook/preview-api',
'version',
'--json',
]);
expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['info', '@storybook/preview-api', 'version', '--json'],
})
);
expect(version).toEqual('5.3.19');
});

Expand All @@ -196,12 +196,12 @@ describe('NPM Proxy', () => {

const version = await npmProxy.latestVersion('@storybook/preview-api', '5.X');

expect(executeCommandSpy).toHaveBeenCalledWith('npm', [
'info',
'@storybook/preview-api',
'versions',
'--json',
]);
expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['info', '@storybook/preview-api', 'versions', '--json'],
})
);
expect(version).toEqual('5.3.19');
});

Expand All @@ -220,12 +220,12 @@ describe('NPM Proxy', () => {

const version = await npmProxy.getVersion('@storybook/angular');

expect(executeCommandSpy).toHaveBeenCalledWith('npm', [
'info',
'@storybook/angular',
'version',
'--json',
]);
expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['info', '@storybook/angular', 'version', '--json'],
})
);
expect(version).toEqual(`^${storybookAngularVersion}`);
});

Expand All @@ -237,12 +237,12 @@ describe('NPM Proxy', () => {

const version = await npmProxy.getVersion('@storybook/react-native');

expect(executeCommandSpy).toHaveBeenCalledWith('npm', [
'info',
'@storybook/react-native',
'version',
'--json',
]);
expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['info', '@storybook/react-native', 'version', '--json'],
})
);
expect(version).toEqual(`^${packageVersion}`);
});
});
Expand Down
33 changes: 21 additions & 12 deletions code/lib/cli/src/js-package-manager/NPMProxy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export class NPMProxy extends JsPackageManager {
installArgs: string[] | undefined;

initPackageJson() {
return this.executeCommand('npm', ['init', '-y']);
return this.executeCommand({ command: 'npm', args: ['init', '-y'] });
}

getRunStorybookCommand(): string {
Expand All @@ -37,7 +37,7 @@ export class NPMProxy extends JsPackageManager {
}

getNpmVersion(): string {
return this.executeCommand('npm', ['--version']);
return this.executeCommand({ command: 'npm', args: ['--version'] });
}

getInstallArgs(): string[] {
Expand All @@ -48,19 +48,21 @@ export class NPMProxy extends JsPackageManager {
}

public runPackageCommand(command: string, args: string[], cwd?: string): string {
return this.executeCommand('npm', ['exec', '--', command, ...args], undefined, cwd);
return this.executeCommand({
command: 'npm',
args: ['exec', '--', command, ...args],
cwd,
});
}

public findInstallations() {
const pipeToNull = platform() === 'win32' ? '2>NUL' : '2>/dev/null';
const commandResult = this.executeCommand(
'npm',
['ls', '--json', '--depth=99', pipeToNull],
undefined,
undefined,
const commandResult = this.executeCommand({
command: 'npm',
args: ['ls', '--json', '--depth=99', pipeToNull],
// ignore errors, because npm ls will exit with code 1 if there are e.g. unmet peer dependencies
true
);
ignoreError: true,
});

try {
const parsedOutput = JSON.parse(commandResult);
Expand Down Expand Up @@ -96,7 +98,11 @@ export class NPMProxy extends JsPackageManager {
protected runRemoveDeps(dependencies: string[]): void {
const args = [...dependencies];

this.executeCommand('npm', ['uninstall', ...this.getInstallArgs(), ...args], 'inherit');
this.executeCommand({
command: 'npm',
args: ['uninstall', ...this.getInstallArgs(), ...args],
stdio: 'inherit',
});
}

protected runGetVersions<T extends boolean>(
Expand All @@ -105,7 +111,10 @@ export class NPMProxy extends JsPackageManager {
): Promise<T extends true ? string[] : string> {
const args = [fetchAllVersions ? 'versions' : 'version', '--json'];

const commandResult = this.executeCommand('npm', ['info', packageName, ...args]);
const commandResult = this.executeCommand({
command: 'npm',
args: ['info', packageName, ...args],
});

try {
const parsedOutput = JSON.parse(commandResult);
Expand Down
Loading

0 comments on commit ef4cc90

Please sign in to comment.