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

Directly execute build-archive-storybook if we can't resolve it #917

Merged
merged 7 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions action-src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ async function run() {
process.chdir(path.join(process.cwd(), workingDir || ''));

const output = await runNode({
options: {
inAction: true,
},
flags: {
allowConsoleErrors: maybe(allowConsoleErrors, false),
autoAcceptChanges: maybe(autoAcceptChanges),
Expand Down
75 changes: 36 additions & 39 deletions node-src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,11 @@ beforeEach(() => {
vi.mock('dns');
vi.mock('execa');

vi.mock('./lib/getE2eBinPath', () => ({
getE2eBinPath: () => 'path/to/bin',
// NOTE: we'd prefer to mock the require.resolve() of `@chromatic-com/playwright/..` but
// vitest doesn't allow you to do that.
const mockedBuildCommand = 'mocked build command';
vi.mock('./lib/getE2eBuildCommand', () => ({
getE2eBuildCommand: () => mockedBuildCommand,
}));

const execaCommand = vi.mocked(execaDefault);
Expand Down Expand Up @@ -291,7 +294,7 @@ vi.mock('./git/git', () => ({
hasPreviousCommit: () => Promise.resolve(true),
getCommit: vi.fn(),
getBranch: () => Promise.resolve('branch'),
getSlug: vi.fn(),
getSlug: vi.fn(),
getVersion: () => Promise.resolve('2.24.1'),
getChangedFiles: () => Promise.resolve(['src/foo.stories.js']),
getRepositoryRoot: () => Promise.resolve(process.cwd()),
Expand All @@ -305,7 +308,7 @@ vi.mock('./git/getParentCommits', () => ({
}));

const getCommit = vi.mocked(git.getCommit);
const getSlug = vi.mocked(git.getSlug)
const getSlug = vi.mocked(git.getSlug);

vi.mock('./lib/emailHash');

Expand Down Expand Up @@ -347,7 +350,7 @@ beforeEach(() => {
committerEmail: '[email protected]',
committerName: 'tester',
});
getSlug.mockResolvedValue('user/repo')
getSlug.mockResolvedValue('user/repo');
});
afterEach(() => {
process.env = processEnv;
Expand Down Expand Up @@ -547,20 +550,14 @@ it('skips building and uploads directly with storybook-build-dir', async () => {
it('builds with playwright with --playwright', async () => {
const ctx = getContext(['--project-token=asdf1234', '--playwright']);
await runAll(ctx);
expect(execaCommand).toHaveBeenCalledWith(
expect.stringMatching(/path\/to\/bin/),
expect.objectContaining({})
);
expect(execaCommand).toHaveBeenCalledWith(mockedBuildCommand, expect.objectContaining({}));
expect(ctx.exitCode).toBe(1);
});

it('builds with cypress with --cypress', async () => {
const ctx = getContext(['--project-token=asdf1234', '--cypress']);
await runAll(ctx);
expect(execaCommand).toHaveBeenCalledWith(
expect.stringMatching(/path\/to\/bin/),
expect.objectContaining({})
);
expect(execaCommand).toHaveBeenCalledWith(mockedBuildCommand, expect.objectContaining({}));
expect(ctx.exitCode).toBe(1);
});

Expand Down Expand Up @@ -797,33 +794,33 @@ it('should upload metadata files if --upload-metadata is passed', async () => {

describe('getGitInfo', () => {
it('should retreive git info', async () => {
const result = await getGitInfo()
const result = await getGitInfo();
expect(result).toMatchObject({
"branch": "branch",
"commit": "commit",
"committedAt": 1234,
"committerEmail": "[email protected]",
"committerName": "tester",
"slug": "user/repo",
"uncommittedHash": "abc123",
"userEmail": "[email protected]",
"userEmailHash": undefined,
})
})
branch: 'branch',
commit: 'commit',
committedAt: 1234,
committerEmail: '[email protected]',
committerName: 'tester',
slug: 'user/repo',
uncommittedHash: 'abc123',
userEmail: '[email protected]',
userEmailHash: undefined,
});
});

it('should still return getInfo if no origin url', async () => {
getSlug.mockRejectedValue(new Error('no origin set'))
const result = await getGitInfo()
getSlug.mockRejectedValue(new Error('no origin set'));
const result = await getGitInfo();
expect(result).toMatchObject({
"branch": "branch",
"commit": "commit",
"committedAt": 1234,
"committerEmail": "[email protected]",
"committerName": "tester",
"slug": "",
"uncommittedHash": "abc123",
"userEmail": "[email protected]",
"userEmailHash": undefined,
})
})
})
branch: 'branch',
commit: 'commit',
committedAt: 1234,
committerEmail: '[email protected]',
committerName: 'tester',
slug: '',
uncommittedHash: 'abc123',
userEmail: '[email protected]',
userEmailHash: undefined,
});
});
});
18 changes: 0 additions & 18 deletions node-src/lib/getE2eBinPath.ts

This file was deleted.

61 changes: 61 additions & 0 deletions node-src/lib/getE2eBuildCommand.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { getCliCommand, Runner, AGENTS } from '@antfu/ni';

import { Context } from '../types';
import missingDependency from '../ui/messages/errors/missingDependency';
import { exitCodes, setExitCode } from './setExitCode';
import { failed } from '../ui/tasks/build';

export const buildBinName = 'build-archive-storybook';

// ni doesn't currently have a "exec" command (equivalent to `npm exec`).
// It has a "download & exec" command (equivalent to `npx`).
// We should probably PR this up to ni
const parseNexec = <Runner>((agent, args) => {
const map: Record<keyof typeof AGENTS, string> = {
npm: 'npm exec {0}',
yarn: 'yarn {0}',
'yarn@berry': 'yarn {0}',
pnpm: 'pnpm exec {0}',
'pnpm@6': 'pnpm exec {0}',
bun: 'bun run {0}',
};

const quote = (arg: string) =>
!arg.startsWith('--') && arg.includes(' ') ? JSON.stringify(arg) : arg;

const command = map[agent];
return command.replace('{0}', args.map(quote).join(' ')).trim();
});

export async function getE2eBuildCommand(
ctx: Context,
flag: 'playwright' | 'cypress',
buildCommandOptions: string[]
) {
console.log('here', ctx.options.inAction);

// The action cannot "peer depend" on or import anything. So instead, we must attempt to exec
// the binary directly.
if (ctx.options.inAction) {
return await getCliCommand(parseNexec, [buildBinName, ...buildCommandOptions], {
programmatic: true,
});
}

const dependencyName = `@chromatic-com/${flag}`;
try {
return [
'node',
require.resolve(`${dependencyName}/bin/${buildBinName}`),
...buildCommandOptions,
].join(' ');
} catch (err) {
if (err.code === 'MODULE_NOT_FOUND') {
ctx.log.error(missingDependency({ dependencyName, flag }));
setExitCode(ctx, exitCodes.MISSING_DEPENDENCY, true);
throw new Error(failed(ctx).output);
}

throw err;
}
}
1 change: 1 addition & 0 deletions node-src/lib/getOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export default function getOptions({
const defaultOptions = {
projectToken: env.CHROMATIC_PROJECT_TOKEN,
fromCI: !!process.env.CI,
inAction: false,
dryRun: false,
debug: false,
autoAcceptChanges: false,
Expand Down
27 changes: 23 additions & 4 deletions node-src/tasks/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import { endActivity, startActivity } from '../ui/components/activity';
import buildFailed from '../ui/messages/errors/buildFailed';
import { failed, initial, pending, skipped, success } from '../ui/tasks/build';
import { getPackageManagerRunCommand } from '../lib/getPackageManager';
import { getE2eBinPath } from '../lib/getE2eBinPath';
import { buildBinName as e2EbuildBinName, getE2eBuildCommand } from '../lib/getE2eBuildCommand';
import missingDependency from '../ui/messages/errors/missingDependency';

export const setSourceDir = async (ctx: Context) => {
if (ctx.options.outputDir) {
Expand Down Expand Up @@ -43,8 +44,11 @@ export const setBuildCommand = async (ctx: Context) => {
].filter(Boolean);

if (ctx.options.playwright || ctx.options.cypress) {
const binPath = getE2eBinPath(ctx, ctx.options.playwright ? 'playwright' : 'cypress');
ctx.buildCommand = ['node', binPath, ...buildCommandOptions].join(' ');
ctx.buildCommand = await getE2eBuildCommand(
ctx,
ctx.options.playwright ? 'playwright' : 'cypress',
buildCommandOptions
);
} else {
ctx.buildCommand = await getPackageManagerRunCommand([
ctx.options.buildScriptName,
Expand Down Expand Up @@ -72,14 +76,29 @@ export const buildStorybook = async (ctx: Context) => {
ctx.log.debug('Running build command:', ctx.buildCommand);
ctx.log.debug('Runtime metadata:', JSON.stringify(ctx.runtimeMetadata, null, 2));

console.log(ctx.buildCommand);
const subprocess = execaCommand(ctx.buildCommand, {
stdio: [null, logFile, logFile],
signal,
env: { NODE_ENV: ctx.env.STORYBOOK_NODE_ENV || 'production' },
});
await Promise.race([subprocess, timeoutAfter(ctx.env.STORYBOOK_BUILD_TIMEOUT)]);
} catch (e) {
// If we tried to run the E2E package's bin directly (due to being in the action)
// and it failed, that means we couldn't find it. This probably means they haven't
// installed the right dependency or run from the right directory
if (
ctx.options.inAction &&
(ctx.options.playwright || ctx.options.cypress) &&
e.message.match(e2EbuildBinName)
) {
const flag = ctx.options.playwright ? 'playwright' : 'cypress';
const dependencyName = `@chromatic-com/${flag}`;
ctx.log.error(missingDependency({ dependencyName, flag, workingDir: process.cwd() }));
ctx.log.debug(e);
setExitCode(ctx, exitCodes.MISSING_DEPENDENCY, true);
throw new Error(failed(ctx).output);
}

signal?.throwIfAborted();

const buildLog = ctx.buildLogFile && readFileSync(ctx.buildLogFile, 'utf8');
Expand Down
1 change: 1 addition & 0 deletions node-src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export interface Options extends Configuration {
traceChanged: boolean | string;
list: Flags['list'];
fromCI: boolean;
inAction: boolean;
skip: boolean | string;
dryRun: Flags['dryRun'];
forceRebuild: boolean | string;
Expand Down
7 changes: 7 additions & 0 deletions node-src/ui/messages/errors/missingDependency.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,10 @@ export default {

export const MissingDependency = () =>
missingDependency({ dependencyName: '@chromatic-com/playwright', flag: 'playwright' });

export const MissingDependencyFromAction = () =>
missingDependency({
dependencyName: '@chromatic-com/playwright',
flag: 'playwright',
workingDir: '/opt/bin/chromatic',
});
15 changes: 14 additions & 1 deletion node-src/ui/messages/errors/missingDependency.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,23 @@ import chalk from 'chalk';
import { dedent } from 'ts-dedent';
import { error, info } from '../../components/icons';

export default ({ dependencyName, flag }: { dependencyName: string; flag: string }) => {
export default ({
dependencyName,
flag,
workingDir,
}: {
dependencyName: string;
flag: string;
workingDir?: string;
}) => {
return dedent(chalk`
${error} Failed to import \`${dependencyName}\`, is it installed in \`package.json\`?

${info} To run \`chromatic --${flag}\` you must have \`${dependencyName}\` installed.
${
workingDir
? `\n${info} Chromatic looked in \`${workingDir}\`. If that's not the right directory, you might need to set the \`workingDir\` option to the action.`
: ''
}
`);
};
Loading