CLI: Support addon-vitest setup when --skip-install is passed#33718
CLI: Support addon-vitest setup when --skip-install is passed#33718valentinpalkovic merged 11 commits into
Conversation
|
View your CI Pipeline Execution ↗ for commit 5f277da
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughPer-addon postinstall now routes to dedicated handlers (Vitest/A11y) and falls back to a default Storybook version when installed version is missing; vitest templates load via runtime ES module imports and package-run APIs gained a Changes
Sequence Diagram(s)sequenceDiagram
participant Initiate as Initiate
participant AddonConfig as AddonConfigurationCommand
participant Versions as "storybook/internal/common"
participant Postinstall as Postinstall Handler
participant PackageMgr as Package Manager
Initiate->>AddonConfig: executeAddonConfiguration(addons, configDir, options)
AddonConfig->>Versions: read versions.storybook
loop per addon
AddonConfig->>Postinstall: call specific handler (vitest / a11y / other) with options
Postinstall->>PackageMgr: runPackageCommand(args..., useRemotePkg?)
PackageMgr-->>Postinstall: execution result
Postinstall-->>AddonConfig: postinstall result (success/failure)
end
AddonConfig-->>Initiate: aggregated results / manual instructions for failures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts`:
- Line 6: The import in AddonConfigurationCommand.ts currently pulls the vitest
postinstall module via a fragile relative path (importing addonVitestPostinstall
from '../../../../addons/vitest/src/postinstall'); replace that with the package
export by importing from '@storybook/addon-vitest/postinstall' so the code uses
the public API (reference symbol: addonVitestPostinstall in
AddonConfigurationCommand.ts) and remains compatible with built/distributed
packages and internal restructuring.
🧹 Nitpick comments (2)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (2)
116-121: Consider using distinct styling for failed vs successful addon indicators.Both success (
✅) and failure (❌) messages useCLI_COLORS.muted, which dims the failure indicator. Users might miss failed addons in the output.♻️ Proposed enhancement for better visual distinction
addons.forEach((addon, index) => { const error = addonResults.get(addon); - logger.log(CLI_COLORS.muted(error ? `❌ ${addon}` : `✅ ${addon}`), { + const message = error + ? CLI_COLORS.error(`❌ ${addon}`) + : CLI_COLORS.muted(`✅ ${addon}`); + logger.log(message, { spacing: index === 0 ? 1 : 0, }); });
64-96: Two cross-package source imports with asymmetric addon handling.This file imports from sibling packages at two locations: line 5 (static import from
addons/vitest/src/postinstall) and line 67 (dynamic import fromcli-storybook/src/postinstallAddon).The asymmetric handling (static for vitest, dynamic for others) is functional but creates inconsistency—both are source imports serving similar purposes (postinstall configuration). Consider standardizing the import approach for clarity.
If additional test addons need postinstall treatment in the future, a registry pattern could improve maintainability:
Registry pattern example
const BUNDLED_POSTINSTALL_HANDLERS: Record<string, PostinstallFn> = { '@storybook/addon-vitest': addonVitestPostinstall, // '@storybook/addon-a11y': addonA11yPostinstall, // when needed };
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 776 KB | 776 KB | 🚨 +3 B 🚨 |
| Dependency size | 67.55 MB | 67.60 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1000 KB | 1.04 MB | 🚨 +42 KB 🚨 |
| Dependency size | 36.92 MB | 36.92 MB | 🚨 +257 B 🚨 |
| Bundle Size Analyzer | node | node |
…ensions. Update template loading mechanism to dynamically import templates as plain text. Enhance error logging in AddonConfigurationCommand.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/addons/vitest/src/updateVitestFile.ts`:
- Around line 3-8: The loadTemplate function currently uses dynamic ESM import
(await import(`../templates/${name}`)) which fails for .txt template files;
replace the dynamic import with reading the template file as text using
fs.readFile and a URL built from import.meta.url (e.g., construct templatePath
via new URL(`../templates/${name}`, import.meta.url) and await
fs.readFile(templatePath, 'utf8')) so loadTemplate returns the plain text
template; update references to templateModule/default to use the read string,
and add a minimal fs import if missing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@code/addons/vitest/src/updateVitestFile.test.ts`:
- Around line 34-36: The forEach callback on Object.entries(replacements) is
using a concise arrow body which causes an implicit return (lint error); change
the callback to a block body or replace the forEach with a for...of loop so the
function does not return a value. Locate the call to
Object.entries(replacements).forEach(...) in updateVitestFile.test.ts and either
convert the arrow to a block (e.g., ([key, value]) => { template =
template.replace(key, normalize(value)); }) or iterate with for (const
[key,value] of Object.entries(replacements)) { template = template.replace(key,
normalize(value)); } ensuring you still use normalize(value) and assign back to
template.
- Around line 25-40: Replace the inline mock factory with a spy-style mock and
move the implementation into test setup: call vi.mock('./updateVitestFile', {
spy: true }) at the top, then in beforeEach use
vi.mocked(loadTemplate).mockImplementation(async (name: string, replacements:
Record<string,string>) => { const templateModule = await
import(`../templates/${name}?raw`); let template = templateModule.default;
Object.entries(replacements).forEach(([key, value]) => { template =
template.replace(key, normalize(value)); }); return template; }); ensure you
reference the real symbol loadTemplate and use vi.mocked(loadTemplate) to type
the mock and change the forEach callback to a block form so it does not return a
value.
…in AddonConfigurationCommand. Enhance logging to inform users about addons that couldn't be configured and provide links to setup instructions.
|
Failed to publish canary version of this pull request, triggered by @valentinpalkovic. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/21587135362 |
…e TypeScript error suppressions in template loading
valentinpalkovic
force-pushed
the
valentin/bundle-addon-vitest-postinstall-into-storybook
branch
from
d7a5fdf to
1efe45f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/addons/vitest/src/postinstall.ts (1)
161-166:⚠️ Potential issue | 🟡 Minor
useRemotePkgwill always befalsehere due to surrounding condition.The
installPlaywrightcall is inside theif (!options.skipInstall)block (line 162), which meansoptions.skipInstallis always falsy at this point. Therefore,useRemotePkg: !!options.skipInstallwill always evaluate tofalse.Either:
- The
useRemotePkgflag is unnecessary here (since we only reach this code when dependencies are installed), or- The logic needs restructuring if Playwright should be installed via remote package in some scenarios.
Given the surrounding context (line 162 checks
!options.skipInstall), option 1 seems correct—you can simplify touseRemotePkg: falseor remove it entirely since the default isfalse.🔧 Proposed fix
await addonVitestService.installPlaywright({ yes: options.yes, - useRemotePkg: !!options.skipInstall, });
🤖 Fix all issues with AI agents
In `@code/core/src/common/js-package-manager/PNPMProxy.ts`:
- Around line 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.
In `@code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts`:
- Around line 65-70: The filter incorrectly checks for .result === 'failed' on
values from addonResults (which are either null or an Error), so update the
filter in the block that calls logManualAddonInstructions to use a truthy check
on addonResults.get(addon) (e.g., addons.filter(addon =>
!!addonResults.get(addon))) or equivalent to select addons with non-null error
entries; this fixes selection of failed addons when calling
logManualAddonInstructions.
| 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, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 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 -30Repository: 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:
runPackageCommandaccepts only basic options andargs(nouseRemotePkg) - Yarn1Proxy:
runPackageCommandaccepts onlyargsand options (nouseRemotePkg) - Yarn2Proxy:
runPackageCommandaccepts onlyargsand options (nouseRemotePkg)
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.
| // some addons failed | ||
| if (hasFailures) { | ||
| this.logManualAddonInstructions( | ||
| addons.filter((addon) => addonResults.get(addon)?.result === 'failed') | ||
| ); | ||
| } |
There was a problem hiding this comment.
Incorrect property access on addonResults values.
The addonResults map stores null for success and the error object for failures (see lines 145, 149). However, the filter on line 68 checks for .result === 'failed', which will never match because the stored values don't have a result property.
The filter should check for truthy values (non-null errors) instead:
🐛 Proposed fix
if (hasFailures) {
this.logManualAddonInstructions(
- addons.filter((addon) => addonResults.get(addon)?.result === 'failed')
+ addons.filter((addon) => addonResults.get(addon) !== null)
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // some addons failed | |
| if (hasFailures) { | |
| this.logManualAddonInstructions( | |
| addons.filter((addon) => addonResults.get(addon)?.result === 'failed') | |
| ); | |
| } | |
| // some addons failed | |
| if (hasFailures) { | |
| this.logManualAddonInstructions( | |
| addons.filter((addon) => addonResults.get(addon) !== null) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts` around
lines 65 - 70, The filter incorrectly checks for .result === 'failed' on values
from addonResults (which are either null or an Error), so update the filter in
the block that calls logManualAddonInstructions to use a truthy check on
addonResults.get(addon) (e.g., addons.filter(addon =>
!!addonResults.get(addon))) or equivalent to select addons with non-null error
entries; this fixes selection of failed addons when calling
logManualAddonInstructions.
|
|
||
| import { resolvePackageDir } from '../../../core/src/shared/utils/module'; | ||
| async function getTemplatePath(name: string) { | ||
| switch (name) { |
There was a problem hiding this comment.
I think we should have a comment here, explaining why we need to do it this way.
…emplatePath function
valentinpalkovic
deleted the
valentin/bundle-addon-vitest-postinstall-into-storybook
branch
valentinpalkovic
added
the
patch:yes
…st-postinstall-into-storybook CLI: Support addon-vitest setup when --skip-install is passed (cherry picked from commit 6d0104a)
github-actions
Bot
added
the
patch:done
3 participants
Closes #32377
What I did
Run @storybook/addon-vitest's and @storybook/addon-a11y's postinstall even with --skip-install during
init. Bundle and call the addon's postinstall directly during init, avoiding runtime resolution failures when deps aren’t installed.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
npx storybook@latest --skip-installand select "Recommended".storybook/vitest.setup.tshas been created, and the Vite config has been modified.Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-33718-sha-6755af19. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33718-sha-6755af19 sandboxor in an existing project withnpx storybook@0.0.0-pr-33718-sha-6755af19 upgrade.More information
0.0.0-pr-33718-sha-6755af19valentin/bundle-addon-vitest-postinstall-into-storybook6755af191770109872)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33718Summary by CodeRabbit
Bug Fixes
New Features
Chores / Refactor
Types
Tests