Skip to content
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
10 changes: 7 additions & 3 deletions code/addons/a11y/src/postinstall.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { JsPackageManagerFactory } from 'storybook/internal/common';
import { JsPackageManagerFactory, versions } from 'storybook/internal/common';

import type { PostinstallOptions } from '../../../lib/cli-storybook/src/add';

export default async function postinstall(options: PostinstallOptions) {
const args = ['storybook', 'automigrate', 'addon-a11y-addon-test'];
const args = [
options.skipInstall ? `storybook@${versions.storybook}` : `storybook`,
'automigrate',
'addon-a11y-addon-test',
];

args.push('--loglevel', 'silent');
args.push('--skip-doctor');
Expand All @@ -25,5 +29,5 @@ export default async function postinstall(options: PostinstallOptions) {
configDir: options.configDir,
});

await jsPackageManager.runPackageCommand({ args });
await jsPackageManager.runPackageCommand({ args, useRemotePkg: !!options.skipInstall });
}
19 changes: 13 additions & 6 deletions code/addons/vitest/src/postinstall.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
formatFileContent,
getProjectRoot,
getStorybookInfo,
versions,
} from 'storybook/internal/common';
import { CLI_COLORS } from 'storybook/internal/node-logger';
import type { StorybookError } from 'storybook/internal/server-errors';
Expand Down Expand Up @@ -161,6 +162,7 @@ export default async function postInstall(options: PostinstallOptions) {
if (!options.skipInstall) {
await addonVitestService.installPlaywright({
yes: options.yes,
useRemotePkg: !!options.skipInstall,
});
} else {
logger.warn(dedent`
Expand Down Expand Up @@ -229,11 +231,11 @@ export default async function postInstall(options: PostinstallOptions) {

const getTemplateName = () => {
if (isVitest4OrNewer) {
return 'vitest.config.4.template.ts';
return 'vitest.config.4.template';
} else if (isVitest3_2To4) {
return 'vitest.config.3.2.template.ts';
return 'vitest.config.3.2.template';
}
return 'vitest.config.template.ts';
return 'vitest.config.template';
};

// If there's an existing workspace file, we update that file to include the Storybook Addon Vitest plugin.
Expand All @@ -249,7 +251,7 @@ export default async function postInstall(options: PostinstallOptions) {
return;
}

const workspaceTemplate = await loadTemplate('vitest.workspace.template.ts', {
const workspaceTemplate = await loadTemplate('vitest.workspace.template', {
EXTENDS_WORKSPACE: viteConfigFile
? relative(dirname(vitestWorkspaceFile), viteConfigFile)
: '',
Expand Down Expand Up @@ -358,7 +360,7 @@ export default async function postInstall(options: PostinstallOptions) {
if (a11yAddon) {
try {
const command = [
'storybook',
options.skipInstall ? `storybook@${versions.storybook}` : `storybook`,
'automigrate',
'addon-a11y-addon-test',
'--loglevel',
Expand All @@ -381,7 +383,12 @@ export default async function postInstall(options: PostinstallOptions) {

await prompt.executeTask(
// TODO: Remove stdio: 'ignore' once we have a way to log the output of the command properly
() => packageManager.runPackageCommand({ args: command, stdio: 'ignore' }),
() =>
packageManager.runPackageCommand({
args: command,
stdio: 'ignore',
useRemotePkg: !!options.skipInstall,
}),
{
intro: 'Setting up a11y addon for @storybook/addon-vitest',
error: 'Failed to setup a11y addon for @storybook/addon-vitest',
Expand Down
5 changes: 5 additions & 0 deletions code/addons/vitest/src/typings.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,8 @@ interface ImportMetaEnv {
interface ImportMeta {
readonly env: ImportMetaEnv;
}

declare module '*?raw' {
const content: string;
export default content;
}
36 changes: 18 additions & 18 deletions code/addons/vitest/src/updateVitestFile.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ vi.mock('../../../core/src/shared/utils/module', () => ({
describe('updateConfigFile', () => {
it('updates vite config file', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -102,7 +102,7 @@ describe('updateConfigFile', () => {

it('supports object notation without defineConfig', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -182,7 +182,7 @@ describe('updateConfigFile', () => {

it('does not support function notation', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -214,7 +214,7 @@ describe('updateConfigFile', () => {

it('adds projects property to test config', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.3.2.template.ts', {
await loadTemplate('vitest.config.3.2.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -293,7 +293,7 @@ describe('updateConfigFile', () => {

it('edits projects property of test config', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.3.2.template.ts', {
await loadTemplate('vitest.config.3.2.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -373,7 +373,7 @@ describe('updateConfigFile', () => {

it('adds workspace property to test config', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -452,7 +452,7 @@ describe('updateConfigFile', () => {

it('adds test property to vite config', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -527,7 +527,7 @@ describe('updateConfigFile', () => {

it('supports mergeConfig with multiple defineConfig calls, finding the one with test', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -613,7 +613,7 @@ describe('updateConfigFile', () => {
});
it('supports mergeConfig without defineConfig calls', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -696,7 +696,7 @@ describe('updateConfigFile', () => {

it('supports mergeConfig without config containing test property', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -772,7 +772,7 @@ describe('updateConfigFile', () => {

it('supports mergeConfig with defineConfig pattern using projects (Vitest 3.2+)', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.3.2.template.ts', {
await loadTemplate('vitest.config.3.2.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -856,7 +856,7 @@ describe('updateConfigFile', () => {

it('appends storybook project to existing test.projects array (no double nesting)', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.3.2.template.ts', {
await loadTemplate('vitest.config.3.2.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -945,7 +945,7 @@ describe('updateConfigFile', () => {

it('extracts coverage config and keeps it at top level when using workspace', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.template.ts', {
await loadTemplate('vitest.config.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -1044,7 +1044,7 @@ describe('updateConfigFile', () => {

it('extracts coverage config and keeps it at top level when using projects', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.config.3.2.template.ts', {
await loadTemplate('vitest.config.3.2.template', {
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
Expand Down Expand Up @@ -1145,7 +1145,7 @@ describe('updateConfigFile', () => {
describe('updateWorkspaceFile', () => {
it('updates vitest workspace file using array syntax', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.workspace.template.ts', {
await loadTemplate('vitest.workspace.template', {
EXTENDS_WORKSPACE: '',
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
Expand Down Expand Up @@ -1201,7 +1201,7 @@ describe('updateWorkspaceFile', () => {

it('updates vitest workspace file using defineWorkspace syntax', async () => {
const source = babel.babelParse(
await loadTemplate('vitest.workspace.template.ts', {
await loadTemplate('vitest.workspace.template', {
EXTENDS_WORKSPACE: '',
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
Expand Down Expand Up @@ -1264,7 +1264,7 @@ describe('loadTemplate', () => {
// Windows-style path with backslashes (need to escape them in JS strings)
const windowsPath = '.\\apps\\frontend-storybook\\.storybook';

const result = await loadTemplate('vitest.config.template.ts', {
const result = await loadTemplate('vitest.config.template', {
CONFIG_DIR: windowsPath,
SETUP_FILE: '.\\apps\\frontend-storybook\\.storybook\\vitest.setup.ts',
});
Expand All @@ -1278,7 +1278,7 @@ describe('loadTemplate', () => {
// Unix-style path with forward slashes
const unixPath = './apps/frontend-storybook/.storybook';

const result = await loadTemplate('vitest.config.template.ts', {
const result = await loadTemplate('vitest.config.template', {
CONFIG_DIR: unixPath,
SETUP_FILE: './apps/frontend-storybook/.storybook/vitest.setup.ts',
});
Expand Down
30 changes: 22 additions & 8 deletions code/addons/vitest/src/updateVitestFile.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,30 @@
import * as fs from 'node:fs/promises';

import type { BabelFile, types as t } from 'storybook/internal/babel';

import { join, normalize } from 'pathe';
import { normalize } from 'pathe';

import { resolvePackageDir } from '../../../core/src/shared/utils/module';
/**
* Each template is imported separately to allow the build system to process the template as raw
* text. A mix of globs and the "?raw" string query is not supported in esbuild
*/
async function getTemplatePath(name: string) {
switch (name) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a comment here, explaining why we need to do it this way.

case 'vitest.config.template':
return import('../templates/vitest.config.template?raw');
case 'vitest.config.4.template':
return import('../templates/vitest.config.4.template?raw');
case 'vitest.config.3.2.template':
return import('../templates/vitest.config.3.2.template?raw');
case 'vitest.workspace.template':
return import('../templates/vitest.workspace.template?raw');
default:
throw new Error(`Unknown template: ${name}`);
}
}

export const loadTemplate = async (name: string, replacements: Record<string, string>) => {
let template = await fs.readFile(
join(resolvePackageDir('@storybook/addon-vitest'), 'templates', name),
'utf8'
);
// Dynamically import the template file as plain text
const templateModule = await getTemplatePath(name);
let template = templateModule.default;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Normalize Windows paths (backslashes) to forward slashes for JavaScript string compatibility
Object.entries(replacements).forEach(
([key, value]) => (template = template.replace(key, normalize(value)))
Expand Down
7 changes: 6 additions & 1 deletion code/core/src/cli/AddonVitestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,11 @@ export class AddonVitestService {
* @returns Array of error messages if installation fails
*/
async installPlaywright(
options: { yes?: boolean } = {}
options: {
yes?: boolean;
/** Is set to true if Storybook didn't install the dependencies yet */
useRemotePkg?: boolean;
} = {}
): Promise<{ errors: string[]; result: 'installed' | 'skipped' | 'aborted' | 'failed' }> {
const errors: string[] = [];

Expand Down Expand Up @@ -148,6 +152,7 @@ export class AddonVitestService {
(signal) =>
this.packageManager.runPackageCommand({
args: playwrightCommand,
useRemotePkg: options.useRemotePkg,
stdio: ['inherit', 'pipe', 'pipe'],
signal,
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ export abstract class JsPackageManager {
stdio?: 'inherit' | 'pipe' | 'ignore'
): ResultPromise;
public abstract runPackageCommand(
options: Omit<ExecuteCommandOptions, 'command'> & { args: string[] }
options: Omit<ExecuteCommandOptions, 'command'> & { args: string[]; useRemotePkg?: boolean }
): ResultPromise;
public abstract findInstallations(pattern?: string[]): Promise<InstallationMetadata | undefined>;
public abstract findInstallations(
Expand Down
8 changes: 6 additions & 2 deletions code/core/src/common/js-package-manager/PNPMProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,15 @@ export class PNPMProxy extends JsPackageManager {

public runPackageCommand({
args,
useRemotePkg = false,
...options
}: Omit<ExecuteCommandOptions, 'command'> & { args: string[] }): ResultPromise {
}: Omit<ExecuteCommandOptions, 'command'> & {
args: string[];
useRemotePkg?: boolean;
}): ResultPromise {
return executeCommand({
command: 'pnpm',
args: ['exec', ...args],
args: [useRemotePkg ? 'dlx' : 'exec', ...args],
...options,
});
}
Comment on lines 80 to 93

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for runPackageCommand implementations in other proxies
echo "=== Checking NPMProxy ==="
rg -n "runPackageCommand" code/core/src/common/js-package-manager/NPMProxy.ts -A 10 | head -30

echo ""
echo "=== Checking Yarn1Proxy ==="
rg -n "runPackageCommand" code/core/src/common/js-package-manager/Yarn1Proxy.ts -A 10 | head -30

echo ""
echo "=== Checking Yarn2Proxy ==="
rg -n "runPackageCommand" code/core/src/common/js-package-manager/Yarn2Proxy.ts -A 10 | head -30

echo ""
echo "=== Checking BunProxy ==="
rg -n "runPackageCommand" code/core/src/common/js-package-manager/BunProxy.ts -A 10 | head -30

Repository: storybookjs/storybook

Length of output: 1209


Address inconsistency: Other package manager proxies lack useRemotePkg support

The PNPMProxy implementation adds useRemotePkg support, but NPMProxy, Yarn1Proxy, and Yarn2Proxy do not have equivalent implementations. This creates an inconsistency across package managers:

  • NPMProxy: runPackageCommand accepts only basic options and args (no useRemotePkg)
  • Yarn1Proxy: runPackageCommand accepts only args and options (no useRemotePkg)
  • Yarn2Proxy: runPackageCommand accepts only args and options (no useRemotePkg)

Either add useRemotePkg support to all proxies, or remove it from PNPMProxy to maintain consistency across the codebase.

🤖 Prompt for AI Agents
In `@code/core/src/common/js-package-manager/PNPMProxy.ts` around lines 80 - 93,
The PNPMProxy.runPackageCommand adds a useRemotePkg option that other proxies
(NPMProxy, Yarn1Proxy, Yarn2Proxy) don’t support; remove the inconsistency by
deleting the useRemotePkg parameter and its usage in
PNPMProxy.runPackageCommand: update the method signature to remove useRemotePkg
and always call executeCommand with args prefixed by the local exec form (i.e.,
use 'exec' rather than the ternary [useRemotePkg ? 'dlx' : 'exec', ...args]),
keeping the function name and return type unchanged.

Expand Down
7 changes: 2 additions & 5 deletions code/lib/cli-storybook/src/automigrate/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { type JsPackageManager } from 'storybook/internal/common';
import { versions } from 'storybook/internal/common';
import { logTracker, logger, prompt } from 'storybook/internal/node-logger';
import { AutomigrateError } from 'storybook/internal/server-errors';
import type { StorybookConfigRaw } from 'storybook/internal/types';
Expand Down Expand Up @@ -56,18 +57,14 @@ export const doAutomigrate = async (options: AutofixOptionsFromCLI) => {
packageManagerName: options.packageManager,
});

if (!versionInstalled) {
throw new Error('Could not determine Storybook version');
}

if (!mainConfigPath) {
throw new Error('Could not determine main config path');
}

const outcome = await automigrate({
...options,
packageManager,
storybookVersion: versionInstalled,
storybookVersion: versionInstalled || versions.storybook,
mainConfigPath,
mainConfig,
previewConfigPath,
Expand Down
Loading
Loading