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

CLI: Gracefully shutdown and cleanup execa child processes #23538

Merged
merged 8 commits into from
Jul 21, 2023
58 changes: 11 additions & 47 deletions code/lib/cli/src/generators/baseGenerator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
extractEslintInfo,
suggestESLintPlugin,
} from '../automigrate/helpers/eslintPlugin';
import { HandledError } from '../HandledError';

const logger = console;

Expand Down Expand Up @@ -185,34 +184,9 @@ export async function baseGenerator(
options: FrameworkOptions = defaultOptions,
framework?: SupportedFrameworks
) {
// This is added so that we can handle the scenario where the user presses Ctrl+C and report a canceled event.
// Given that there are subprocesses running as part of this function, we need to handle the signal ourselves otherwise it might run into race conditions.
// TODO: This should be revisited once we have a better way to handle this.
let isNodeProcessExiting = false;
const setNodeProcessExiting = () => {
isNodeProcessExiting = true;
};
process.on('SIGINT', setNodeProcessExiting);

const isStorybookInMonorepository = packageManager.isStorybookInMonorepo();
const shouldApplyRequireWrapperOnPackageNames = isStorybookInMonorepository || pnp;

const stopIfExiting = async <T>(callback: () => Promise<T>) => {
if (isNodeProcessExiting) {
throw new HandledError('Canceled by the user');
}

try {
return await callback();
} catch (error) {
if (isNodeProcessExiting) {
throw new HandledError('Canceled by the user');
}

throw error;
}
};

const {
extraAddons: extraAddonPackages,
extraPackages,
Expand Down Expand Up @@ -308,9 +282,7 @@ export async function baseGenerator(
indent: 2,
text: `Getting the correct version of ${packages.length} packages`,
}).start();
const versionedPackages = await stopIfExiting(async () =>
packageManager.getVersionedPackages(packages)
);
const versionedPackages = await packageManager.getVersionedPackages(packages);
versionedPackagesSpinner.succeed();

const depsToInstall = [...versionedPackages];
Expand Down Expand Up @@ -369,9 +341,7 @@ export async function baseGenerator(
indent: 2,
text: 'Installing Storybook dependencies',
}).start();
await stopIfExiting(async () =>
packageManager.addDependencies({ ...npmOptions, packageJson }, depsToInstall)
);
await packageManager.addDependencies({ ...npmOptions, packageJson }, depsToInstall);
addDependenciesSpinner.succeed();
}

Expand Down Expand Up @@ -429,24 +399,18 @@ export async function baseGenerator(
});

if (addScripts) {
await stopIfExiting(async () =>
packageManager.addStorybookCommandInScripts({
port: 6006,
})
);
await packageManager.addStorybookCommandInScripts({
port: 6006,
});
}

if (addComponents) {
const templateLocation = hasFrameworkTemplates(framework) ? framework : rendererId;
await stopIfExiting(async () =>
copyTemplateFiles({
renderer: templateLocation,
packageManager,
language,
destination: componentsDestinationPath,
})
);
await copyTemplateFiles({
renderer: templateLocation,
packageManager,
language,
destination: componentsDestinationPath,
});
}

process.off('SIGINT', setNodeProcessExiting);
}
135 changes: 74 additions & 61 deletions code/lib/cli/src/initiate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,18 @@ const projectTypeInquirer = async (
process.exit(0);
};

async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise<void> {
async function doInitiate(
options: CommandOptions,
pkg: PackageJson
): Promise<
| {
shouldRunDev: true;
projectType: ProjectType;
packageManager: JsPackageManager;
storybookCommand: string;
}
| { shouldRunDev: false }
> {
let { packageManager: pkgMgr } = options;
if (options.useNpm) {
useNpmWarning();
Expand Down Expand Up @@ -317,7 +328,7 @@ async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise<vo
}

if (!options.disableTelemetry) {
telemetry('init', { projectType });
await telemetry('init', { projectType });
}

if (projectType === ProjectType.REACT_NATIVE) {
Expand All @@ -330,14 +341,17 @@ async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise<vo
logger.log('\n For more in information, see the github readme:\n');
logger.log(chalk.cyan('https://github.com/storybookjs/react-native'));
logger.log();
} else {
const storybookCommand =
projectType === ProjectType.ANGULAR
? `ng run ${installResult.projectName}:storybook`
: packageManager.getRunStorybookCommand();
logger.log(
boxen(
dedent`

return { shouldRunDev: false };
}

const storybookCommand =
projectType === ProjectType.ANGULAR
? `ng run ${installResult.projectName}:storybook`
: packageManager.getRunStorybookCommand();
logger.log(
boxen(
dedent`
Storybook was successfully installed in your project! 🎉
To run Storybook manually, run ${chalk.yellow(
chalk.bold(storybookCommand)
Expand All @@ -346,65 +360,64 @@ async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise<vo
Wanna know more about Storybook? Check out ${chalk.cyan('https://storybook.js.org/')}
Having trouble or want to chat? Join us at ${chalk.cyan('https://discord.gg/storybook/')}
`,
{ borderStyle: 'round', padding: 1, borderColor: '#F1618C' }
)
);

const shouldRunDev = process.env.CI !== 'true' && process.env.IN_STORYBOOK_SANDBOX !== 'true';
if (shouldRunDev) {
logger.log('\nRunning Storybook');

try {
const isReactWebProject =
projectType === ProjectType.REACT_SCRIPTS ||
projectType === ProjectType.REACT ||
projectType === ProjectType.WEBPACK_REACT ||
projectType === ProjectType.REACT_PROJECT ||
projectType === ProjectType.NEXTJS;

const flags = [];

// npm needs extra -- to pass flags to the command
if (packageManager.type === 'npm') {
flags.push('--');
}

if (isReactWebProject) {
flags.push('--initial-path=/onboarding');
}

flags.push('--quiet');

// instead of calling 'dev' automatically, we spawn a subprocess so that it gets
// executed directly in the user's project directory. This avoid potential issues
// with packages running in npxs' node_modules
packageManager.runPackageCommandSync(
storybookCommand.replace(/^yarn /, ''),
flags,
undefined,
'inherit'
);
} catch (e) {
const isCtrlC =
e.message.includes('Command failed with exit code 129') &&
e.message.includes('CTRL+C') &&
e.message.includes('SIGINT');
if (!isCtrlC) {
// only throw if it's not ctrl + c
throw e;
}
}
}
}
{ borderStyle: 'round', padding: 1, borderColor: '#F1618C' }
)
);

return {
shouldRunDev: process.env.CI !== 'true' && process.env.IN_STORYBOOK_SANDBOX !== 'true',
projectType,
packageManager,
storybookCommand,
};
}

export async function initiate(options: CommandOptions, pkg: PackageJson): Promise<void> {
await withTelemetry(
const initiateResult = await withTelemetry(
'init',
{
cliOptions: options,
printError: (err) => !err.handled && logger.error(err),
},
() => doInitiate(options, pkg)
);

if (initiateResult.shouldRunDev) {
const { projectType, packageManager, storybookCommand } = initiateResult;
logger.log('\nRunning Storybook');

try {
const isReactWebProject =
projectType === ProjectType.REACT_SCRIPTS ||
projectType === ProjectType.REACT ||
projectType === ProjectType.WEBPACK_REACT ||
projectType === ProjectType.REACT_PROJECT ||
projectType === ProjectType.NEXTJS;

const flags = [];

// npm needs extra -- to pass flags to the command
if (packageManager.type === 'npm') {
flags.push('--');
}

if (isReactWebProject) {
flags.push('--initial-path=/onboarding');
}

flags.push('--quiet');

// instead of calling 'dev' automatically, we spawn a subprocess so that it gets
// executed directly in the user's project directory. This avoid potential issues
// with packages running in npxs' node_modules
packageManager.runPackageCommandSync(
storybookCommand.replace(/^yarn /, ''),
flags,
undefined,
'inherit'
);
} catch (e) {
// Do nothing here, as the command above will spawn a `storybook dev` process which does the error handling already. Else, the error will get bubbled up and sent to crash reports twice
}
}
}
2 changes: 2 additions & 0 deletions code/lib/cli/src/js-package-manager/JsPackageManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,7 @@ export abstract class JsPackageManager {
stdio: stdio ?? 'pipe',
encoding: 'utf-8',
shell: true,
cleanup: true,
env,
...execaOptions,
});
Expand Down Expand Up @@ -529,6 +530,7 @@ export abstract class JsPackageManager {
stdio: stdio ?? 'pipe',
encoding: 'utf-8',
shell: true,
cleanup: true,
env,
...execaOptions,
});
Expand Down
5 changes: 4 additions & 1 deletion code/lib/cli/src/repro-generators/scripts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,10 @@ const addLocalPackageResolutions = async ({ cwd }: Options) => {
const packageJsonPath = path.join(cwd, 'package.json');
const packageJson = await readJSON(packageJsonPath);
const workspaceDir = path.join(__dirname, '..', '..', '..', '..', '..');
const { stdout } = await command('yarn workspaces list --json', { cwd: workspaceDir });
const { stdout } = await command('yarn workspaces list --json', {
cwd: workspaceDir,
cleanup: true,
});

const workspaces = JSON.parse(`[${stdout.split('\n').join(',')}]`);

Expand Down
23 changes: 15 additions & 8 deletions code/lib/core-server/src/withTelemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,20 @@ export async function withTelemetry<T>(
options: TelemetryOptions,
run: () => Promise<T>
): Promise<T> {
let canceled = false;

async function cancelTelemetry() {
canceled = true;
if (!options.cliOptions.disableTelemetry) {
await telemetry('canceled', { eventType }, { stripMetadata: true, immediate: true });
}

process.exit(0);
}

if (eventType === 'init') {
// We catch Ctrl+C user interactions to be able to detect a cancel event
process.on('SIGINT', async () => {
if (!options.cliOptions.disableTelemetry) {
await telemetry('canceled', { eventType }, { stripMetadata: true, immediate: true });
}

process.exit(0);
});
process.on('SIGINT', cancelTelemetry);
}

if (!options.cliOptions.disableTelemetry)
Expand All @@ -126,7 +131,7 @@ export async function withTelemetry<T>(
try {
return await run();
} catch (error) {
if (error?.message === 'Canceled by the user') {
if (canceled) {
return undefined;
}

Expand All @@ -135,5 +140,7 @@ export async function withTelemetry<T>(
await sendTelemetryError(error, eventType, options);

throw error;
} finally {
process.off('SIGINIT', cancelTelemetry);
}
}
1 change: 1 addition & 0 deletions scripts/build-package.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ async function run() {
cwd,
buffer: false,
shell: true,
cleanup: true,
env: {
NODE_ENV: 'production',
},
Expand Down
1 change: 1 addition & 0 deletions scripts/check-package.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ async function run() {
cwd,
buffer: false,
shell: true,
cleanup: true,
env: {
NODE_ENV: 'production',
},
Expand Down
5 changes: 4 additions & 1 deletion scripts/utils/exec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ export const execaCommand = async (
const execa = await getExeca();
// We await here because execaCommand returns a promise, but that's not what the user expects
// eslint-disable-next-line @typescript-eslint/return-await
return await execa.execaCommand(command, options);
return await execa.execaCommand(command, {
cleanup: true,
...options,
});
};

export const exec = async (
Expand Down