From 45c9801a8826f09d8fe6694e3ed43133137ab802 Mon Sep 17 00:00:00 2001 From: Cody Kaup Date: Wed, 23 Oct 2024 12:09:49 -0500 Subject: [PATCH 1/7] Add support for --build-command --- action-src/main.ts | 2 ++ action.yml | 3 +++ node-src/lib/getConfiguration.ts | 1 + node-src/lib/getOptions.ts | 10 ++++++++++ node-src/lib/parseArguments.ts | 2 ++ node-src/tasks/build.test.ts | 1 + node-src/tasks/build.ts | 10 ++++++++++ node-src/types.ts | 2 ++ 8 files changed, 31 insertions(+) diff --git a/action-src/main.ts b/action-src/main.ts index 5c188267c..c7d124291 100755 --- a/action-src/main.ts +++ b/action-src/main.ts @@ -104,6 +104,7 @@ async function run() { const autoAcceptChanges = getInput('autoAcceptChanges'); const branchName = getInput('branchName'); const buildScriptName = getInput('buildScriptName'); + const buildCommand = getInput('buildCommand'); const configFile = getInput('configFile'); const cypress = getInput('cypress'); const debug = getInput('debug'); @@ -158,6 +159,7 @@ async function run() { autoAcceptChanges: maybe(autoAcceptChanges), branchName: maybe(branchName), buildScriptName: maybe(buildScriptName), + buildCommand: maybe(buildCommand), configFile: maybe(configFile), cypress: maybe(cypress), debug: maybe(debug), diff --git a/action.yml b/action.yml index 629f84809..696d15e9a 100755 --- a/action.yml +++ b/action.yml @@ -21,6 +21,9 @@ inputs: buildScriptName: description: 'The npm script that builds your Storybook [build-storybook]' required: false + buildCommand: + description: 'The command that builds your Storybook (when your build command does not exist in "scripts" of your package.json)' + required: false configFile: description: 'Path from where to load the Chromatic config JSON file.' cypress: diff --git a/node-src/lib/getConfiguration.ts b/node-src/lib/getConfiguration.ts index b9846f668..b61b47c7f 100644 --- a/node-src/lib/getConfiguration.ts +++ b/node-src/lib/getConfiguration.ts @@ -29,6 +29,7 @@ const configurationSchema = z ignoreLastBuildOnBranch: z.string(), buildScriptName: z.string(), + buildCommand: z.string(), playwright: z.boolean(), cypress: z.boolean(), outputDir: z.string(), diff --git a/node-src/lib/getOptions.ts b/node-src/lib/getOptions.ts index 39e413cc1..4f3fe0554 100644 --- a/node-src/lib/getOptions.ts +++ b/node-src/lib/getOptions.ts @@ -92,6 +92,7 @@ export default function getOptions(ctx: InitialContext): Options { preserveMissingSpecs: undefined, buildScriptName: undefined, + buildCommand: undefined, playwright: undefined, cypress: undefined, outputDir: undefined, @@ -158,6 +159,7 @@ export default function getOptions(ctx: InitialContext): Options { flags.preserveMissing || typeof flags.only === 'string' ? true : undefined, buildScriptName: flags.buildScriptName, + buildCommand: flags.buildCommand, playwright: trueIfSet(flags.playwright), cypress: trueIfSet(flags.cypress), outputDir: takeLast(flags.outputDir), @@ -290,6 +292,10 @@ export default function getOptions(ctx: InitialContext): Options { throw new Error(incompatibleOptions(['--junit-report', '--exit-once-uploaded'])); } + if (potentialOptions.buildScriptName && potentialOptions.buildCommand) { + throw new Error(incompatibleOptions(['--build-script-name', '--build-command'])); + } + if ( typeof potentialOptions.junitReport === 'string' && path.extname(potentialOptions.junitReport) !== '.xml' @@ -315,6 +321,10 @@ export default function getOptions(ctx: InitialContext): Options { return options; } + if (potentialOptions.buildCommand) { + return options; + } + if (isE2EBuild(options)) { return options; } diff --git a/node-src/lib/parseArguments.ts b/node-src/lib/parseArguments.ts index 67289f5bf..17af6ec40 100644 --- a/node-src/lib/parseArguments.ts +++ b/node-src/lib/parseArguments.ts @@ -24,6 +24,7 @@ export default function parseArguments(argv: string[]) { Storybook options --build-script-name, -b [name] The npm script that builds your Storybook we should take snapshots against. Use this if your Storybook build script is named differently. [build-storybook] + --build-command The command that builds your Storybook we should take snapshots against. Use this if your Storybook build command does not exist in "scripts" of your package.json (like using NX). --output-dir, -o Relative path to target directory for building your Storybook, in case you want to preserve it. Otherwise a temporary directory is used if possible. --storybook-build-dir, -d If you have already built your Storybook, provide the path to the static build directory. @@ -82,6 +83,7 @@ export default function parseArguments(argv: string[]) { // Storybook options buildScriptName: { type: 'string', alias: 'b' }, + buildCommand: { type: 'string' }, outputDir: { type: 'string', alias: 'o', isMultiple: true }, storybookBuildDir: { type: 'string', alias: 'd', isMultiple: true }, diff --git a/node-src/tasks/build.test.ts b/node-src/tasks/build.test.ts index bb2676ddd..94bb71e11 100644 --- a/node-src/tasks/build.test.ts +++ b/node-src/tasks/build.test.ts @@ -68,6 +68,7 @@ describe('setBuildCommand', () => { const ctx = { sourceDir: './source-dir/', options: { buildScriptName: 'build:storybook' }, + flags: {}, storybook: { version: '6.1.0' }, git: {}, } as any; diff --git a/node-src/tasks/build.ts b/node-src/tasks/build.ts index 41573ce06..31a1fc618 100644 --- a/node-src/tasks/build.ts +++ b/node-src/tasks/build.ts @@ -45,6 +45,16 @@ export const setBuildCommand = async (ctx: Context) => { ctx.git.changedFiles && webpackStatsSupported && `--webpack-stats-json=${ctx.sourceDir}`, ].filter((c): c is string => !!c); + const buildCommand = ctx.flags.buildCommand || ctx.options.buildCommand; + if (buildCommand) { + ctx.buildCommand = `${buildCommand} ${buildCommandOptions + // There is a bug in NX that outputs an invalid Storybook if the `--output-dir` flag is passed. + // Therefore, we need to skip that until it's fixed: https://github.com/nrwl/nx/issues/28594 + .filter((c) => !c.includes('--output-dir')) + .join(' ')}`; + return; + } + if (isE2EBuild(ctx.options)) { ctx.buildCommand = await getE2EBuildCommand( ctx, diff --git a/node-src/types.ts b/node-src/types.ts index ff6ea80d0..8e34eab82 100644 --- a/node-src/types.ts +++ b/node-src/types.ts @@ -13,6 +13,7 @@ export interface Flags { // Storybook options buildScriptName?: string; + buildCommand?: string; outputDir?: string[]; storybookBuildDir?: string[]; @@ -102,6 +103,7 @@ export interface Options extends Configuration { originalArgv: string[]; buildScriptName: Flags['buildScriptName']; + buildCommand: Flags['buildCommand']; playwright: Flags['playwright']; cypress: Flags['cypress']; outputDir: string; From 6df2a04f04a6f30f849090e6992de5ead65e1b89 Mon Sep 17 00:00:00 2001 From: Cody Kaup Date: Wed, 23 Oct 2024 12:11:02 -0500 Subject: [PATCH 2/7] Pass context to invalid directory message --- node-src/tasks/build.ts | 14 +++++++------- node-src/tasks/upload.ts | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/node-src/tasks/build.ts b/node-src/tasks/build.ts index 31a1fc618..2b004dab1 100644 --- a/node-src/tasks/build.ts +++ b/node-src/tasks/build.ts @@ -40,18 +40,18 @@ export const setBuildCommand = async (ctx: Context) => { ctx.log.warn('Storybook version 6.2.0 or later is required to use the --only-changed flag'); } + const buildCommand = ctx.flags.buildCommand || ctx.options.buildCommand; + const buildCommandOptions = [ - `--output-dir=${ctx.sourceDir}`, + // NOTE: There is a bug in NX that outputs an invalid Storybook if the `--output-dir` flag is + // passed. Therefore, we need to skip that until it's fixed: https://github.com/nrwl/nx/issues/28594 + // When that's fixed, we can remove the `!buildCommand &&` below. + !buildCommand && `--output-dir=${ctx.sourceDir}`, ctx.git.changedFiles && webpackStatsSupported && `--webpack-stats-json=${ctx.sourceDir}`, ].filter((c): c is string => !!c); - const buildCommand = ctx.flags.buildCommand || ctx.options.buildCommand; if (buildCommand) { - ctx.buildCommand = `${buildCommand} ${buildCommandOptions - // There is a bug in NX that outputs an invalid Storybook if the `--output-dir` flag is passed. - // Therefore, we need to skip that until it's fixed: https://github.com/nrwl/nx/issues/28594 - .filter((c) => !c.includes('--output-dir')) - .join(' ')}`; + ctx.buildCommand = `${buildCommand} ${buildCommandOptions.join(' ')}`; return; } diff --git a/node-src/tasks/upload.ts b/node-src/tasks/upload.ts index 8fcd17645..dd85af329 100644 --- a/node-src/tasks/upload.ts +++ b/node-src/tasks/upload.ts @@ -61,7 +61,7 @@ function getPathsInDirectory(ctx: Context, rootDirectory: string, dirname = '.') }); } catch (err) { ctx.log.debug(err); - throw new Error(invalid({ sourceDir: rootDirectory } as any, err).output); + throw new Error(invalid({ ...ctx, sourceDir: rootDirectory }, err).output); } } From bb00c03d50d59b41b240d5983b63085b88f92deb Mon Sep 17 00:00:00 2001 From: Cody Kaup Date: Wed, 23 Oct 2024 12:11:09 -0500 Subject: [PATCH 3/7] Update build failed message to account for --build-command --- node-src/ui/messages/errors/buildFailed.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/node-src/ui/messages/errors/buildFailed.ts b/node-src/ui/messages/errors/buildFailed.ts index bfda9f574..aa48c1b9a 100644 --- a/node-src/ui/messages/errors/buildFailed.ts +++ b/node-src/ui/messages/errors/buildFailed.ts @@ -11,15 +11,20 @@ export default ( { message }, buildLog?: string ) => { - const { buildScriptName } = options; + const { buildScriptName, buildCommand: buildCommandOption } = options; const lines = buildLog?.split(EOL).filter((line) => line && !line.startsWith('')) || []; + const commandToBuild = buildScriptName || buildCommandOption; + const suggestedRunCommands = buildScriptName + ? `{bold npm run ${commandToBuild}} or {bold yarn ${commandToBuild}}` + : `{bold ${commandToBuild}}`; + return [ dedent(chalk` - The CLI tried to run your {bold ${buildScriptName}} script, but the command failed. This indicates a problem with your Storybook. Here's what to do: + The CLI tried to run your {bold ${commandToBuild}} script, but the command failed. This indicates a problem with your Storybook. Here's what to do: - Check the Storybook build log printed below. - - Run {bold npm run ${buildScriptName}} or {bold yarn ${buildScriptName}} yourself and make sure it outputs a valid Storybook by opening the generated {bold index.html} in your browser. + - Run ${suggestedRunCommands} yourself and make sure it outputs a valid Storybook by opening the generated {bold index.html} in your browser. - Review the build-storybook CLI options at ${link( 'https://storybook.js.org/docs/configurations/cli-options/#for-build-storybook' )} From 95b6db12c27e4ab221835734453a07c489a01350 Mon Sep 17 00:00:00 2001 From: Cody Kaup Date: Wed, 23 Oct 2024 14:23:03 -0500 Subject: [PATCH 4/7] Fix and add tests --- node-src/tasks/build.test.ts | 55 ++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/node-src/tasks/build.test.ts b/node-src/tasks/build.test.ts index 94bb71e11..41762dc5f 100644 --- a/node-src/tasks/build.test.ts +++ b/node-src/tasks/build.test.ts @@ -12,31 +12,41 @@ vi.mock('@antfu/ni'); const command = vi.mocked(execaCommand); const getCliCommand = vi.mocked(getCliCommandDefault); +const baseContext = { options: {}, flags: {} } as any; + beforeEach(() => { command.mockClear(); }); describe('setSourceDir', () => { it('sets a random temp directory path on the context', async () => { - const ctx = { options: {}, storybook: { version: '5.0.0' } } as any; + const ctx = { ...baseContext, storybook: { version: '5.0.0' } } as any; await setSourceDirectory(ctx); expect(ctx.sourceDir).toMatch(/chromatic-/); }); it('falls back to the default output dir for older Storybooks', async () => { - const ctx = { options: {}, storybook: { version: '4.0.0' } } as any; + const ctx = { ...baseContext, storybook: { version: '4.0.0' } } as any; await setSourceDirectory(ctx); expect(ctx.sourceDir).toBe('storybook-static'); }); it('uses the outputDir option if provided', async () => { - const ctx = { options: { outputDir: 'storybook-out' }, storybook: { version: '5.0.0' } } as any; + const ctx = { + ...baseContext, + options: { outputDir: 'storybook-out' }, + storybook: { version: '5.0.0' }, + } as any; await setSourceDirectory(ctx); expect(ctx.sourceDir).toBe('storybook-out'); }); it('uses the outputDir option if provided, even for older Storybooks', async () => { - const ctx = { options: { outputDir: 'storybook-out' }, storybook: { version: '4.0.0' } } as any; + const ctx = { + ...baseContext, + options: { outputDir: 'storybook-out' }, + storybook: { version: '4.0.0' }, + } as any; await setSourceDirectory(ctx); expect(ctx.sourceDir).toBe('storybook-out'); }); @@ -47,6 +57,7 @@ describe('setBuildCommand', () => { getCliCommand.mockReturnValue(Promise.resolve('npm run build:storybook')); const ctx = { + ...baseContext, sourceDir: './source-dir/', options: { buildScriptName: 'build:storybook' }, storybook: { version: '6.2.0' }, @@ -66,11 +77,11 @@ describe('setBuildCommand', () => { getCliCommand.mockReturnValue(Promise.resolve('yarn run build:storybook')); const ctx = { + ...baseContext, sourceDir: './source-dir/', options: { buildScriptName: 'build:storybook' }, - flags: {}, - storybook: { version: '6.1.0' }, - git: {}, + storybook: { version: '6.2.0' }, + git: { changedFiles: ['./index.js'] }, } as any; await setBuildCommand(ctx); @@ -86,10 +97,11 @@ describe('setBuildCommand', () => { getCliCommand.mockReturnValue(Promise.resolve('pnpm run build:storybook')); const ctx = { + ...baseContext, sourceDir: './source-dir/', options: { buildScriptName: 'build:storybook' }, - storybook: { version: '6.1.0' }, - git: {}, + storybook: { version: '6.2.0' }, + git: { changedFiles: ['./index.js'] }, } as any; await setBuildCommand(ctx); @@ -101,8 +113,27 @@ describe('setBuildCommand', () => { expect(ctx.buildCommand).toEqual('pnpm run build:storybook'); }); + it('uses --build-command, if set', async () => { + getCliCommand.mockReturnValue(Promise.resolve('npm run build:storybook')); + + const ctx = { + ...baseContext, + sourceDir: './source-dir/', + options: { buildCommand: 'nx run my-app:build-storybook' }, + storybook: { version: '6.2.0' }, + git: { changedFiles: ['./index.js'] }, + } as any; + await setBuildCommand(ctx); + + expect(getCliCommand).not.toHaveBeenCalled(); + expect(ctx.buildCommand).toEqual( + 'nx run my-app:build-storybook --webpack-stats-json=./source-dir/' + ); + }); + it('warns if --only-changes is not supported', async () => { const ctx = { + ...baseContext, sourceDir: './source-dir/', options: { buildScriptName: 'build:storybook' }, storybook: { version: '6.1.0' }, @@ -119,6 +150,7 @@ describe('setBuildCommand', () => { describe('buildStorybook', () => { it('runs the build command', async () => { const ctx = { + ...baseContext, buildCommand: 'npm run build:storybook --script-args', env: { STORYBOOK_BUILD_TIMEOUT: 1000 }, log: { debug: vi.fn() }, @@ -135,6 +167,7 @@ describe('buildStorybook', () => { it('fails when build times out', async () => { const ctx = { + ...baseContext, buildCommand: 'npm run build:storybook --script-args', options: { buildScriptName: '' }, env: { STORYBOOK_BUILD_TIMEOUT: 0 }, @@ -147,6 +180,7 @@ describe('buildStorybook', () => { it('passes NODE_ENV=production', async () => { const ctx = { + ...baseContext, buildCommand: 'npm run build:storybook --script-args', env: { STORYBOOK_BUILD_TIMEOUT: 1000 }, log: { debug: vi.fn() }, @@ -161,6 +195,7 @@ describe('buildStorybook', () => { it('allows overriding NODE_ENV with STORYBOOK_NODE_ENV', async () => { const ctx = { + ...baseContext, buildCommand: 'npm run build:storybook --script-args', env: { STORYBOOK_BUILD_TIMEOUT: 1000, STORYBOOK_NODE_ENV: 'test' }, log: { debug: vi.fn() }, @@ -195,6 +230,7 @@ describe('buildStorybook E2E', () => { 'fails with missing dependency error when error message is $name', async ({ error }) => { const ctx = { + ...baseContext, buildCommand: 'npm exec build-archive-storybook', options: { buildScriptName: '', playwright: true }, env: { STORYBOOK_BUILD_TIMEOUT: 0 }, @@ -213,6 +249,7 @@ describe('buildStorybook E2E', () => { it('fails with generic error message when not missing dependency error', async () => { const ctx = { + ...baseContext, buildCommand: 'npm exec build-archive-storybook', options: { buildScriptName: '', playwright: true }, env: { STORYBOOK_BUILD_TIMEOUT: 0 }, From 122c59701a260024367626a8c2f47ed96aa5fd95 Mon Sep 17 00:00:00 2001 From: Cody Kaup Date: Wed, 23 Oct 2024 14:37:43 -0500 Subject: [PATCH 5/7] Globally reset mocks between each test --- vitest.config.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/vitest.config.ts b/vitest.config.ts index 46cb0b4f3..93bb3ba32 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -3,6 +3,7 @@ import { configDefaults, coverageConfigDefaults, defineConfig } from 'vitest/con export default defineConfig({ test: { exclude: [...configDefaults.exclude, '**/getParentCommits.test.ts'], + clearMocks: true, // Clear all mocks between each test coverage: { provider: 'v8', exclude: [ From 6c57be157d0dd593e6ab7f0fd0def9f771812276 Mon Sep 17 00:00:00 2001 From: Cody Kaup Date: Wed, 23 Oct 2024 14:57:45 -0500 Subject: [PATCH 6/7] Add story and fix chalk output --- node-src/ui/messages/errors/buildFailed.stories.ts | 12 ++++++++++++ node-src/ui/messages/errors/buildFailed.ts | 4 ++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/node-src/ui/messages/errors/buildFailed.stories.ts b/node-src/ui/messages/errors/buildFailed.stories.ts index 72eb28e6e..c37747b74 100644 --- a/node-src/ui/messages/errors/buildFailed.stories.ts +++ b/node-src/ui/messages/errors/buildFailed.stories.ts @@ -59,3 +59,15 @@ export const BuildFailed = () => { message: 'Command failed with exit code 1' }, buildLog ); + +export const BuildFailedWithCommand = () => + buildFailed( + { + options: { buildCommand: 'nx run my-app:build-storybook' }, + buildCommand, + buildLogFile: '/path/to/project/build-storybook.log', + runtimeMetadata, + } as any, + { message: 'Command failed with exit code 1' }, + buildLog + ); diff --git a/node-src/ui/messages/errors/buildFailed.ts b/node-src/ui/messages/errors/buildFailed.ts index aa48c1b9a..300736b46 100644 --- a/node-src/ui/messages/errors/buildFailed.ts +++ b/node-src/ui/messages/errors/buildFailed.ts @@ -16,8 +16,8 @@ export default ( const commandToBuild = buildScriptName || buildCommandOption; const suggestedRunCommands = buildScriptName - ? `{bold npm run ${commandToBuild}} or {bold yarn ${commandToBuild}}` - : `{bold ${commandToBuild}}`; + ? chalk`{bold npm run ${commandToBuild}} or {bold yarn ${commandToBuild}}` + : chalk`{bold ${commandToBuild}}`; return [ dedent(chalk` From a1c1456671f6ffa2f8c920d6e0a7700c5554cf82 Mon Sep 17 00:00:00 2001 From: Cody Kaup Date: Thu, 24 Oct 2024 11:50:43 -0500 Subject: [PATCH 7/7] Enforce --output-dir with --build-command --- node-src/lib/getOptions.ts | 6 ++++++ node-src/lib/parseArguments.ts | 2 +- node-src/tasks/build.ts | 3 --- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/node-src/lib/getOptions.ts b/node-src/lib/getOptions.ts index 4f3fe0554..947556fa0 100644 --- a/node-src/lib/getOptions.ts +++ b/node-src/lib/getOptions.ts @@ -296,6 +296,12 @@ export default function getOptions(ctx: InitialContext): Options { throw new Error(incompatibleOptions(['--build-script-name', '--build-command'])); } + // --build-command can put the built Storybook anywhere. Rather than reading through the value, + // we require `--output-dir` to avoid the issue. + if (potentialOptions.buildCommand && !potentialOptions.outputDir) { + throw new Error(dependentOption('--build-command', '--output-dir')); + } + if ( typeof potentialOptions.junitReport === 'string' && path.extname(potentialOptions.junitReport) !== '.xml' diff --git a/node-src/lib/parseArguments.ts b/node-src/lib/parseArguments.ts index 17af6ec40..573dcee80 100644 --- a/node-src/lib/parseArguments.ts +++ b/node-src/lib/parseArguments.ts @@ -24,7 +24,7 @@ export default function parseArguments(argv: string[]) { Storybook options --build-script-name, -b [name] The npm script that builds your Storybook we should take snapshots against. Use this if your Storybook build script is named differently. [build-storybook] - --build-command The command that builds your Storybook we should take snapshots against. Use this if your Storybook build command does not exist in "scripts" of your package.json (like using NX). + --build-command The command that builds your Storybook we should take snapshots against. Use this if your Storybook build command does not exist in "scripts" of your package.json (like using NX). Requires --output-dir. --output-dir, -o Relative path to target directory for building your Storybook, in case you want to preserve it. Otherwise a temporary directory is used if possible. --storybook-build-dir, -d If you have already built your Storybook, provide the path to the static build directory. diff --git a/node-src/tasks/build.ts b/node-src/tasks/build.ts index 2b004dab1..1f4f6bb45 100644 --- a/node-src/tasks/build.ts +++ b/node-src/tasks/build.ts @@ -43,9 +43,6 @@ export const setBuildCommand = async (ctx: Context) => { const buildCommand = ctx.flags.buildCommand || ctx.options.buildCommand; const buildCommandOptions = [ - // NOTE: There is a bug in NX that outputs an invalid Storybook if the `--output-dir` flag is - // passed. Therefore, we need to skip that until it's fixed: https://github.com/nrwl/nx/issues/28594 - // When that's fixed, we can remove the `!buildCommand &&` below. !buildCommand && `--output-dir=${ctx.sourceDir}`, ctx.git.changedFiles && webpackStatsSupported && `--webpack-stats-json=${ctx.sourceDir}`, ].filter((c): c is string => !!c);