From 3b0509fb9bef26b62530d5ca7b18039fb981c9a7 Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Thu, 20 Jul 2023 13:50:21 +0200 Subject: [PATCH 1/6] Await telemetry init event --- code/lib/cli/src/initiate.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/lib/cli/src/initiate.ts b/code/lib/cli/src/initiate.ts index 7922004d6d66..500c76043856 100644 --- a/code/lib/cli/src/initiate.ts +++ b/code/lib/cli/src/initiate.ts @@ -317,7 +317,7 @@ async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise Date: Thu, 20 Jul 2023 13:51:12 +0200 Subject: [PATCH 2/6] Gracefully shutdown and cleanup execa child processes Execa child processes should be cleanup up as soon as the parent process quits. During initialization the "cancel" event should properly be sent and it should not lead to error events sent. --- code/lib/cli/src/generators/baseGenerator.ts | 58 ++++--------------- .../js-package-manager/JsPackageManager.ts | 16 ++++- code/lib/cli/src/repro-generators/scripts.ts | 5 +- code/lib/core-server/src/withTelemetry.ts | 2 +- scripts/build-package.js | 1 + scripts/check-package.js | 1 + scripts/utils/exec.ts | 5 +- 7 files changed, 36 insertions(+), 52 deletions(-) diff --git a/code/lib/cli/src/generators/baseGenerator.ts b/code/lib/cli/src/generators/baseGenerator.ts index 03c667e9dac2..3ec3edec38bf 100644 --- a/code/lib/cli/src/generators/baseGenerator.ts +++ b/code/lib/cli/src/generators/baseGenerator.ts @@ -17,7 +17,6 @@ import { extractEslintInfo, suggestESLintPlugin, } from '../automigrate/helpers/eslintPlugin'; -import { HandledError } from '../HandledError'; const logger = console; @@ -178,31 +177,6 @@ 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 stopIfExiting = async (callback: () => Promise) => { - 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, @@ -291,9 +265,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]; @@ -352,9 +324,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(); } @@ -383,24 +353,18 @@ export async function baseGenerator( await configurePreview({ frameworkPreviewParts, storybookConfigFolder, language, rendererId }); 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); } diff --git a/code/lib/cli/src/js-package-manager/JsPackageManager.ts b/code/lib/cli/src/js-package-manager/JsPackageManager.ts index aea7bf07f17f..db47ab0d63c4 100644 --- a/code/lib/cli/src/js-package-manager/JsPackageManager.ts +++ b/code/lib/cli/src/js-package-manager/JsPackageManager.ts @@ -451,13 +451,19 @@ export abstract class JsPackageManager { stdio: stdio ?? 'pipe', encoding: 'utf-8', shell: true, + cleanup: true, env, ...execaOptions, }); return commandResult.stdout ?? ''; } catch (err) { - if (ignoreError !== true) { + if ( + ignoreError !== true && + err.killed !== true && + err.isCanceled !== true && + !err.message?.includes('Command was killed with SIGINT') + ) { throw err; } return ''; @@ -484,13 +490,19 @@ export abstract class JsPackageManager { stdio: stdio ?? 'pipe', encoding: 'utf-8', shell: true, + cleanup: true, env, ...execaOptions, }); return commandResult.stdout ?? ''; } catch (err) { - if (ignoreError !== true) { + if ( + ignoreError !== true && + err.killed !== true && + err.isCanceled !== true && + !err.message?.includes('Command was killed with SIGINT') + ) { throw err; } return ''; diff --git a/code/lib/cli/src/repro-generators/scripts.ts b/code/lib/cli/src/repro-generators/scripts.ts index fc601a6c7ece..2aa59fc2ee40 100644 --- a/code/lib/cli/src/repro-generators/scripts.ts +++ b/code/lib/cli/src/repro-generators/scripts.ts @@ -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(',')}]`); diff --git a/code/lib/core-server/src/withTelemetry.ts b/code/lib/core-server/src/withTelemetry.ts index ef231fe83c24..2c46be9bad01 100644 --- a/code/lib/core-server/src/withTelemetry.ts +++ b/code/lib/core-server/src/withTelemetry.ts @@ -126,7 +126,7 @@ export async function withTelemetry( try { return await run(); } catch (error) { - if (error?.message === 'Canceled by the user') { + if (error?.message.includes('Command was killed with SIGINT')) { return undefined; } diff --git a/scripts/build-package.js b/scripts/build-package.js index 93c480891a59..584d765a21e1 100644 --- a/scripts/build-package.js +++ b/scripts/build-package.js @@ -132,6 +132,7 @@ async function run() { cwd, buffer: false, shell: true, + cleanup: true, env: { NODE_ENV: 'production', }, diff --git a/scripts/check-package.js b/scripts/check-package.js index bf2d95a8fbd7..7fc8bc88f198 100644 --- a/scripts/check-package.js +++ b/scripts/check-package.js @@ -103,6 +103,7 @@ async function run() { cwd, buffer: false, shell: true, + cleanup: true, env: { NODE_ENV: 'production', }, diff --git a/scripts/utils/exec.ts b/scripts/utils/exec.ts index cf2f5e4bd624..6280f65ff9cf 100644 --- a/scripts/utils/exec.ts +++ b/scripts/utils/exec.ts @@ -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 ( From fdc8f8184c9ab29e5394448dbf95a6e122474a62 Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Fri, 21 Jul 2023 09:15:45 +0200 Subject: [PATCH 3/6] Implement robust solution to not report SIGINT as error --- code/lib/core-server/src/withTelemetry.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/code/lib/core-server/src/withTelemetry.ts b/code/lib/core-server/src/withTelemetry.ts index 2c46be9bad01..5542e4ba6d4b 100644 --- a/code/lib/core-server/src/withTelemetry.ts +++ b/code/lib/core-server/src/withTelemetry.ts @@ -109,9 +109,12 @@ export async function withTelemetry( options: TelemetryOptions, run: () => Promise ): Promise { + let canceled = false; + if (eventType === 'init') { // We catch Ctrl+C user interactions to be able to detect a cancel event process.on('SIGINT', async () => { + canceled = true; if (!options.cliOptions.disableTelemetry) { await telemetry('canceled', { eventType }, { stripMetadata: true, immediate: true }); } @@ -126,7 +129,7 @@ export async function withTelemetry( try { return await run(); } catch (error) { - if (error?.message.includes('Command was killed with SIGINT')) { + if (canceled) { return undefined; } From bd4616348b9c6bf82e8c5582ab653010ef6813ce Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Fri, 21 Jul 2023 11:17:08 +0200 Subject: [PATCH 4/6] Cancel process properly inside telemetry HoC --- code/lib/cli/src/initiate.ts | 133 ++++++++++-------- .../js-package-manager/JsPackageManager.ts | 32 ++--- code/lib/core-server/src/withTelemetry.ts | 20 +-- 3 files changed, 95 insertions(+), 90 deletions(-) diff --git a/code/lib/cli/src/initiate.ts b/code/lib/cli/src/initiate.ts index 500c76043856..32aad556577c 100644 --- a/code/lib/cli/src/initiate.ts +++ b/code/lib/cli/src/initiate.ts @@ -239,7 +239,18 @@ const projectTypeInquirer = async ( process.exit(0); }; -async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise { +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(); @@ -330,14 +341,17 @@ async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise { - await withTelemetry( + const initiateResult = await withTelemetry( 'init', { cliOptions: options, @@ -407,4 +381,43 @@ export async function initiate(options: CommandOptions, pkg: PackageJson): Promi }, () => 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) { + // + } + } } diff --git a/code/lib/cli/src/js-package-manager/JsPackageManager.ts b/code/lib/cli/src/js-package-manager/JsPackageManager.ts index db47ab0d63c4..f07724803a8f 100644 --- a/code/lib/cli/src/js-package-manager/JsPackageManager.ts +++ b/code/lib/cli/src/js-package-manager/JsPackageManager.ts @@ -445,29 +445,17 @@ export abstract class JsPackageManager { cwd?: string; ignoreError?: boolean; }): string { - try { - const commandResult = execaCommandSync(command, args, { - cwd: cwd ?? this.cwd, - stdio: stdio ?? 'pipe', - encoding: 'utf-8', - shell: true, - cleanup: true, - env, - ...execaOptions, - }); + const commandResult = execaCommandSync(command, args, { + cwd: cwd ?? this.cwd, + stdio: stdio ?? 'pipe', + encoding: 'utf-8', + shell: true, + cleanup: true, + env, + ...execaOptions, + }); - return commandResult.stdout ?? ''; - } catch (err) { - if ( - ignoreError !== true && - err.killed !== true && - err.isCanceled !== true && - !err.message?.includes('Command was killed with SIGINT') - ) { - throw err; - } - return ''; - } + return commandResult.stdout ?? ''; } public async executeCommand({ diff --git a/code/lib/core-server/src/withTelemetry.ts b/code/lib/core-server/src/withTelemetry.ts index 5542e4ba6d4b..82bd7488dd21 100644 --- a/code/lib/core-server/src/withTelemetry.ts +++ b/code/lib/core-server/src/withTelemetry.ts @@ -111,16 +111,18 @@ export async function withTelemetry( ): Promise { 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 () => { - canceled = true; - if (!options.cliOptions.disableTelemetry) { - await telemetry('canceled', { eventType }, { stripMetadata: true, immediate: true }); - } - - process.exit(0); - }); + process.on('SIGINT', cancelTelemetry); } if (!options.cliOptions.disableTelemetry) @@ -138,5 +140,7 @@ export async function withTelemetry( await sendTelemetryError(error, eventType, options); throw error; + } finally { + process.off('SIGINIT', cancelTelemetry); } } From e2231e03a0411f005c1853d6f5ee2172a36e96da Mon Sep 17 00:00:00 2001 From: Yann Braga Date: Fri, 21 Jul 2023 12:40:40 +0200 Subject: [PATCH 5/6] Update code/lib/cli/src/initiate.ts --- code/lib/cli/src/initiate.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/lib/cli/src/initiate.ts b/code/lib/cli/src/initiate.ts index 32aad556577c..0b2a75a4a488 100644 --- a/code/lib/cli/src/initiate.ts +++ b/code/lib/cli/src/initiate.ts @@ -417,7 +417,7 @@ export async function initiate(options: CommandOptions, pkg: PackageJson): Promi '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 } } } From 2d3e76a54400f3cffbc86f0117efc539228d7e67 Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Fri, 21 Jul 2023 13:20:18 +0200 Subject: [PATCH 6/6] Fix error handling for execaCommand in JsPackageManager --- .../js-package-manager/JsPackageManager.ts | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/code/lib/cli/src/js-package-manager/JsPackageManager.ts b/code/lib/cli/src/js-package-manager/JsPackageManager.ts index f07724803a8f..0536ce8f7121 100644 --- a/code/lib/cli/src/js-package-manager/JsPackageManager.ts +++ b/code/lib/cli/src/js-package-manager/JsPackageManager.ts @@ -445,17 +445,24 @@ export abstract class JsPackageManager { cwd?: string; ignoreError?: boolean; }): string { - const commandResult = execaCommandSync(command, args, { - cwd: cwd ?? this.cwd, - stdio: stdio ?? 'pipe', - encoding: 'utf-8', - shell: true, - cleanup: true, - env, - ...execaOptions, - }); + try { + const commandResult = execaCommandSync(command, args, { + cwd: cwd ?? this.cwd, + stdio: stdio ?? 'pipe', + encoding: 'utf-8', + shell: true, + cleanup: true, + env, + ...execaOptions, + }); - return commandResult.stdout ?? ''; + return commandResult.stdout ?? ''; + } catch (err) { + if (ignoreError !== true) { + throw err; + } + return ''; + } } public async executeCommand({ @@ -485,12 +492,7 @@ export abstract class JsPackageManager { return commandResult.stdout ?? ''; } catch (err) { - if ( - ignoreError !== true && - err.killed !== true && - err.isCanceled !== true && - !err.message?.includes('Command was killed with SIGINT') - ) { + if (ignoreError !== true) { throw err; } return '';