Builder-Vite: Centralize Vite plugins for builder-vite and addon-vitest#33819
Builder-Vite: Centralize Vite plugins for builder-vite and addon-vitest#33819valentinpalkovic merged 30 commits intonextfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit 40177d0
☁️ Nx Cloud last updated this comment at |
- Removed commented-out import for `getProjectAnnotations` in `codegen-modern-iframe-script.ts`. - Eliminated the direct call to `storybookRuntimePlugin` in `preset.ts`, now included in `vite-config.ts` for better organization. - Cleaned up `sandbox-parts.ts` by removing unnecessary `beforeAll` import while maintaining functionality for CSF4 support.
- Updated the `storybookOptimizeDepsPlugin` to directly resolve the configuration with an empty object instead of destructuring from the provided config. This change streamlines the plugin's logic for better clarity and maintainability.
- Introduced a new `optimizeDeps.ts` file containing the `getOptimizeDeps` function to handle dependency optimization for Vite. - Updated `vite-server.ts` to utilize `getOptimizeDeps` for improved dependency inclusion in the Vite server configuration. - Refactored `storybookOptimizeDepsPlugin` to streamline the inclusion of extra dependencies without redundant resolution logic. - Removed unused asyncFilter function from `storybookOptimizeDepsPlugin` for cleaner codebase.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors builder-vite and addon-vitest plugin/preset architecture: introduces viteCorePlugins and multiple new builder-vite Vite plugins (config, entry, optimize-deps, project-annotations, runtime), removes sanitizeEnvVars, simplifies optimizeDeps and codegen paths, and updates vitest plugin to fetch and merge core Vite plugins. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/builders/builder-vite/src/vite-config.test.ts (1)
1-44:⚠️ Potential issue | 🟡 MinorAlign Vitest mocks with the repo's spy-mocking rules
vi.mock('vite', ...)is missing thespy: trueoption. Additionally, mock implementations must be set in abeforeEachblock, not inline within test cases.Required changes
-import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -vi.mock('vite', async (importOriginal) => ({ - ...(await importOriginal<typeof import('vite')>()), - loadConfigFromFile: vi.fn(async () => ({})), - defaultClientConditions: undefined, -})); +vi.mock( + 'vite', + async (importOriginal) => ({ + ...(await importOriginal<typeof import('vite')>()), + loadConfigFromFile: vi.fn(async () => ({})), + defaultClientConditions: undefined, + }), + { spy: true } +); const loadConfigFromFileMock = vi.mocked(loadConfigFromFile); + +beforeEach(() => { + loadConfigFromFileMock.mockResolvedValue({ + config: {}, + path: '', + dependencies: [], + }); +}); @@ - loadConfigFromFileMock.mockReturnValueOnce( - Promise.resolve({ - config: {}, - path: '', - dependencies: [], - }) - );Per the spy-mocking rules: mocks must use
spy: true, all mock behaviors must be inbeforeEachblocks, and inline mock implementations within test cases must be avoided.
🤖 Fix all issues with AI agents
In `@code/builders/builder-vite/src/codegen-project-annotations.ts`:
- Around line 95-97: The current hash function (hash) in
codegen-project-annotations.ts is weak (sums char codes) and can collide for
permutations; replace it with a robust deterministic string hash (e.g., FNV-1a
or a cryptographic hash like SHA-1/SHA-256 via crypto.createHash) so returned
values are hex digests rather than simple integers, and update any callers
expecting the old numeric value to accept the new string digest; ensure the
implementation remains deterministic and performant for filenames and adjust
tests if they assert the previous numeric output.
In `@code/builders/builder-vite/src/plugins/storybook-config-plugin.ts`:
- Around line 53-69: The envPrefix logic in async config(config) of
storybook-config-plugin.ts currently discards user-defined config.envPrefix;
change it to merge and deduplicate user prefixes with 'STORYBOOK_' instead of
replacing them: if config.envPrefix exists, produce an array that includes all
entries from config.envPrefix plus 'STORYBOOK_' (only add 'STORYBOOK_' if not
already present), otherwise keep the fallback ['VITE_', 'STORYBOOK_']; ensure
the final value is assigned to envPrefix in the returned config object.
In `@code/builders/builder-vite/src/plugins/storybook-runtime-plugin.ts`:
- Around line 57-66: In storybookRuntimePlugin, presets.apply is incorrectly
typed as presets.apply<Promise<Builder_EnvsRaw>>('env') causing a double Promise
so awaited envs is still a Promise; change the generic to the raw type (e.g.,
presets.apply<Builder_EnvsRaw>('env') or
presets.apply<Record<string,string>>('env')) so await returns the actual env
object, then stringifyProcessEnvs(envs, config.envPrefix) and Object.keys(envs)
will operate on the real object; update the call site in storybookRuntimePlugin
and ensure the envs variable usage (stringifyProcessEnvs, Object.keys) assumes
the non-Promise shape.
In `@code/builders/builder-vite/src/preset.ts`:
- Around line 13-30: The JSDoc lists "Docgen plugin" and "Runtime plugin" but
the preset's returned plugins array in preset.ts does not include
implementations for those; either remove or update those entries in the comment
to match reality, or add the corresponding plugin instances to the returned
array (e.g. the docgen plugin and the runtime transformation plugin) and ensure
their factory names match existing exports (use the actual plugin factory names
used elsewhere in the repo, e.g. docgenPlugin / runtimePlugin or the equivalent
functions), updating the comment text if you choose to implement them so the
docs and the returned plugins remain consistent.
- Around line 37-42: The computed externals object in preset.ts (built from
globalsNameReferenceMap and conditionally adding '@storybook/addon-docs/blocks'
when build?.test?.disableBlocks is true) is never used; update
storybookRuntimePlugin to accept an externals parameter (change its signature in
storybook-runtime-plugin.ts to accept externals: Record<string,string>), modify
its internal references to use that externals argument instead of directly
reading globalsNameReferenceMap, and then pass the computed externals from
preset.ts into the call to storybookRuntimePlugin so the disableBlocks override
is applied.
🧹 Nitpick comments (3)
code/builders/builder-vite/src/vite-server.ts (1)
15-25: DeduplicateoptimizeDeps.includewhen merging
getOptimizeDepsalready appendscommonCfg.optimizeDeps?.include; merging the same list again can introduce duplicates and redundant pre-bundling. Consider deduping (or relying solely onoptimizeDeps.include).♻️ Suggested change
const config: InlineConfig & { server: ServerOptions } = { ...commonCfg, // Set up dev server optimizeDeps: { ...commonCfg.optimizeDeps, - include: [...(commonCfg.optimizeDeps?.include || []), ...optimizeDeps.include], + include: [ + ...new Set([...(commonCfg.optimizeDeps?.include ?? []), ...optimizeDeps.include]), + ], },code/addons/vitest/src/vitest-plugin/index.ts (1)
33-237: Consider flatteningviteCorePluginsoutput to handle the fullPluginOptiontype specWhile the current
viteCorePluginsimplementation returns a flat array, its type signature (PluginOption[]) permits nested arrays and falsy entries per Vite's type definitions. The direct cast(corePlugins as Plugin[])bypasses this type safety. For defensive robustness, flatten and filter before spreading.♻️ Suggested change
- const plugins: Plugin[] = [ - ...(corePlugins as Plugin[]), - ...(await withoutVitePlugins(viteConfigFromStorybook.plugins ?? [], pluginsToIgnore)), - ]; + const corePlugins = (corePlugins ?? []).flat().filter(Boolean) as Plugin[]; + + const plugins: Plugin[] = [ + ...corePlugins, + ...(await withoutVitePlugins(viteConfigFromStorybook.plugins ?? [], pluginsToIgnore)), + ];code/builders/builder-vite/src/codegen-project-annotations.ts (1)
56-69: Consider handling the empty annotations edge case.If all entries in
previewAnnotationsareundefinedand get filtered out,previewAnnotationURLswill be empty. This causesvariables[variables.length - 1](line 71) to returnundefined, which would generate invalid code for the CSF4 path.While
generateProjectAnnotationsCodealways appendspreviewOrConfigFile(which may beundefined), and CSF4 detection requires a valid preview file, an explicit guard could improve robustness forgenerateProjectAnnotationsCodeFromPreviewsas a public API.🛡️ Optional: Add early return for empty annotations
export function generateProjectAnnotationsCodeFromPreviews(options: { previewAnnotations: (PreviewAnnotation | undefined)[]; projectRoot: string; frameworkName: string; isCsf4: boolean; }) { const { projectRoot } = options; const previewAnnotationURLs = options.previewAnnotations .filter((path) => path !== undefined) .map((path) => processPreviewAnnotation(path, projectRoot)); + if (previewAnnotationURLs.length === 0) { + return dedent` + export function getProjectAnnotations() { + return {}; + } + `.trim(); + } + const variables: string[] = [];
| function hash(value: string) { | ||
| return value.split('').reduce((acc, char) => acc + char.charCodeAt(0), 0); | ||
| } |
There was a problem hiding this comment.
Weak hash function can produce collisions.
This hash function sums character codes, so permutations of the same characters produce identical hashes (e.g., "abc" and "cba" both hash to 294). While the filename prefix provides some disambiguation, consider a more robust approach if uniqueness is important.
🔧 Suggested improvement using a simple string hash
function hash(value: string) {
- return value.split('').reduce((acc, char) => acc + char.charCodeAt(0), 0);
+ // Simple djb2-style hash for better distribution
+ return value.split('').reduce((acc, char) => ((acc << 5) - acc + char.charCodeAt(0)) | 0, 5381) >>> 0;
}🤖 Prompt for AI Agents
In `@code/builders/builder-vite/src/codegen-project-annotations.ts` around lines
95 - 97, The current hash function (hash) in codegen-project-annotations.ts is
weak (sums char codes) and can collide for permutations; replace it with a
robust deterministic string hash (e.g., FNV-1a or a cryptographic hash like
SHA-1/SHA-256 via crypto.createHash) so returned values are hex digests rather
than simple integers, and update any callers expecting the old numeric value to
accept the new string digest; ensure the implementation remains deterministic
and performant for filenames and adjust tests if they assert the previous
numeric output.
code/builders/builder-vite/src/plugins/storybook-runtime-plugin.ts
Outdated
Show resolved
Hide resolved
| const build = await options.presets.apply('build'); | ||
| const externals: Record<string, string> = { ...globalsNameReferenceMap }; | ||
|
|
||
| const coreOptions = await options.presets.apply('core'); | ||
| if (build?.test?.disableBlocks) { | ||
| externals['@storybook/addon-docs/blocks'] = '__STORYBOOK_BLOCKS_EMPTY_MODULE__'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the full preset.ts file to understand the context
cat -n code/builders/builder-vite/src/preset.tsRepository: storybookjs/storybook
Length of output: 2919
🏁 Script executed:
# Search for runtime plugin and externals usage patterns
rg -n 'storybookRuntimePlugin|externals' code/builders/builder-vite/src/ -A 2 -B 2Repository: storybookjs/storybook
Length of output: 6798
🏁 Script executed:
# Check if there are any runtime plugins in the codebase that handle externals
rg -n 'runtimePlugin' code/builders/builder-vite/ -A 3Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Look for how other presets handle externals or similar configurations
fd -t f 'preset.ts' code/builders/ -x grep -l 'externals' {}Repository: storybookjs/storybook
Length of output: 105
🏁 Script executed:
# Read the storybook-runtime-plugin.ts to understand how it's meant to be used
cat -n code/builders/builder-vite/src/plugins/storybook-runtime-plugin.ts | head -80Repository: storybookjs/storybook
Length of output: 3190
🏁 Script executed:
# Check vite-config.ts to see how storybookRuntimePlugin and corePlugins are called
cat -n code/builders/builder-vite/src/vite-config.ts | sed -n '70,85p'Repository: storybookjs/storybook
Length of output: 729
🏁 Script executed:
# Check if externals parameter should be passed to storybookRuntimePlugin
rg -A 10 'function storybookRuntimePlugin' code/builders/builder-vite/src/plugins/storybook-runtime-plugin.tsRepository: storybookjs/storybook
Length of output: 554
🏁 Script executed:
# Check if disableBlocks is used elsewhere and what it's supposed to do
rg -n 'disableBlocks' code/builders/builder-vite/ -B 2 -A 2Repository: storybookjs/storybook
Length of output: 902
🏁 Script executed:
# Check the StorybookRuntimePluginOptions interface more carefully
cat -n code/builders/builder-vite/src/plugins/storybook-runtime-plugin.ts | sed -n '16,41p'Repository: storybookjs/storybook
Length of output: 1179
🏁 Script executed:
# Check how externalGlobalsPlugin uses the externals parameter
cat -n code/builders/builder-vite/src/plugins/external-globals-plugin.ts | sed -n '35,70p'Repository: storybookjs/storybook
Length of output: 1797
🏁 Script executed:
# Final confirmation: check all callers of storybookRuntimePlugin
rg -n 'storybookRuntimePlugin(' code/builders/builder-vite/ --type tsRepository: storybookjs/storybook
Length of output: 161
🏁 Script executed:
# Verify the signature hasn't been updated elsewhere
grep -r 'export.*function storybookRuntimePlugin' code/builders/builder-vite/Repository: storybookjs/storybook
Length of output: 215
🏁 Script executed:
# Check if StorybookRuntimePluginOptions interface is used anywhere
rg -n 'StorybookRuntimePluginOptions' code/builders/builder-vite/Repository: storybookjs/storybook
Length of output: 183
externals is computed but never used—this prevents the disableBlocks override from being applied.
The externals object is built from globalsNameReferenceMap with a conditional override for @storybook/addon-docs/blocks when tests disable blocks, but it's never passed to the runtime plugin. The storybookRuntimePlugin() function is hardcoded to use globalsNameReferenceMap directly (line 58 of storybook-runtime-plugin.ts), with no way to accept the computed externals including the blocks override. Update the storybookRuntimePlugin signature to accept externals as a parameter and pass the computed value from preset.ts.
🤖 Prompt for AI Agents
In `@code/builders/builder-vite/src/preset.ts` around lines 37 - 42, The computed
externals object in preset.ts (built from globalsNameReferenceMap and
conditionally adding '@storybook/addon-docs/blocks' when
build?.test?.disableBlocks is true) is never used; update storybookRuntimePlugin
to accept an externals parameter (change its signature in
storybook-runtime-plugin.ts to accept externals: Record<string,string>), modify
its internal references to use that externals argument instead of directly
reading globalsNameReferenceMap, and then pass the computed externals from
preset.ts into the call to storybookRuntimePlugin so the disableBlocks override
is applied.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 402 KB | 391 KB | 🎉 -11 KB 🎉 |
| Dependency size | 338 KB | 338 KB | 0 B |
| Bundle Size Analyzer | Link | Link |
This reverts commit 9915f3d.
…server" This reverts commit 59342f0.
- Moved the external globals logic into the `storybookRuntimePlugin` function. - Removed redundant external globals mapping from `viteCorePlugins`. - Added conditional handling for `@storybook/addon-docs/blocks` based on build configuration.
- Removed the `VIRTUAL_PROJECT_ANNOTATIONS_FILE` from `SB_VIRTUAL_FILES`. - Updated the `VIRTUAL_ID` in the `storybook-project-annotations-plugin` to directly use the virtual file path string instead of the constant.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/builders/builder-vite/src/preset.ts`:
- Around line 30-33: Remove the unused existing parameter from the
viteCorePlugins signature and all call sites (they currently pass []), and
update the JSDoc for viteCorePlugins to remove references to "Docgen plugin" and
"Runtime plugin" and instead accurately describe only the plugins actually
returned by the function; locate the function named viteCorePlugins and its
callers to change the signature and calls, and edit the comment block above
viteCorePlugins to list the correct returned plugins.
🧹 Nitpick comments (1)
code/builders/builder-vite/src/plugins/storybook-config-plugin.ts (1)
59-71: Consider returning a config object instead of mutating directly.The plugin mutates
config.server.fs.allowviapush(). While Vite tolerates this, returning a merged config object is the recommended pattern for better predictability and debugging.♻️ Suggested refactor
{ name: 'storybook:allow-storybook-dir', enforce: 'post', config(config) { // If there is NO allow list then Vite allows anything in the root directory. // If there IS an allow list then Vite only allows the listed directories. // We add the storybook config directory only if there's already an allow list, // to avoid disallowing the root unless it's already restricted. if (config?.server?.fs?.allow) { - config.server.fs.allow.push(options.configDir); + return { + server: { + fs: { + allow: [...config.server.fs.allow, options.configDir], + }, + }, + }; } }, },
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.ts`:
- Around line 25-32: The plugin currently overwrites optimizeDeps.entries with
only Storybook entries; instead merge Storybook entries with any user-provided
entries so existing Vite inference or explicit hints aren't lost. Update the
object returned in the function that builds optimizeDeps to combine
getUniqueImportPaths(index) with config.optimizeDeps?.entries (e.g., concat or
dedupe) and set optimizeDeps.entries to that merged array, while leaving the
include merge logic (extraOptimizeDeps + config.optimizeDeps?.include)
unchanged.
code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.ts
Show resolved
Hide resolved
- Removed unused `readFileSync` import in `preset.ts`. - Simplified imports in `envs.ts` by removing the `Options` type. - Cleaned up imports in `storybook-config-plugin.ts` by removing the unused `resolvePathInStorybookCache` and adjusting the `Plugin` import.
…ons plugin - Simplified the `generateModernIframeScriptCode` function by removing unused parameters and code related to preview annotations. - Updated the `generateModernIframeScriptCodeFromPreviews` function to utilize the new `getProjectAnnotations` import. - Enhanced the `storybookProjectAnnotationsPlugin` to include HMR support for project annotations, allowing for dynamic updates during development.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/builders/builder-vite/src/codegen-modern-iframe-script.ts`:
- Line 7: The generated import uses RESOLVED_VIRTUAL_ID (which contains the
internal '\0' prefix) instead of the public virtual ID; update the code that
builds the import string to use the public ID by either importing and using
VIRTUAL_ID from the plugin or by calling
getOriginalVirtualModuleId(RESOLVED_VIRTUAL_ID) to strip the prefix, similar to
how SB_VIRTUAL_FILES.* is used elsewhere, and replace occurrences of
RESOLVED_VIRTUAL_ID in the generated source with that public ID.
- Renamed the `RESOLVED_VIRTUAL_ID` constant to `PROJECT_ANNOTATIONS_VIRTUAL_ID` for clarity. - Updated import statements in `codegen-modern-iframe-script.ts` to reflect the new naming. - Adjusted the order of plugin inclusion in `preset.ts` to ensure `storybookProjectAnnotationsPlugin` is added correctly. - Cleaned up the `storybook-project-annotations-plugin.ts` by reordering the declaration of `VIRTUAL_ID` and `RESOLVED_VIRTUAL_ID` for better readability.
bea9b8d to
2d0adaa
Compare
|
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/21949475020 |
- Updated the `entries` property in the `storybookOptimizeDepsPlugin` to allow for both string and array formats, improving flexibility in dependency optimization.
| plugins.push({ | ||
| name: 'storybook:env-plugin', | ||
| config(config) { | ||
| const envDefines = stringifyProcessEnvs(envs, config.envPrefix); |
There was a problem hiding this comment.
take an extra look about this on vitest integration
…e plugin integration
…tProjectAnnotations function
|
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/22184468133 |
Closes #33813
What I did
This PR refactors the @storybook/builder-vite plugin architecture to move core logic into a more modular, plugin-based structure.
Key Changes
1. Modularization of Vite Plugins
Previously, much of the Vite configuration (resolve conditions, environment variables, fs-allow rules) was hardcoded within the builder's vite-config.ts. These have been extracted into self-contained plugins:
storybookConfigPlugin: Handles resolve conditions (storybook, stories, test), envPrefix (VITE_, STORYBOOK_), and filesystem access.storybookProjectAnnotationsPlugin: Manages the virtual module for composed project annotations (getProjectAnnotations).storybookRuntimePlugin: Handles external globals and environment variable injection.2. Introduction of viteCorePlugins Preset
A new
viteCorePluginspreset has been added to builder-vite/src/preset.ts. (We don't want to useviteFinalto be able to clearly distinguish what is a shareable core plugin and what is config contributed by other addons/frameworks/user config - This was requested by Jeppe). This serves as the single source of truth for the Vite setup required to run Storybook code, and it can now be applied by both the standard builder and the addon-vitest.3. Refined Virtual Module Handling
The project annotations logic has been moved out of the monolithic
codeGeneratorPlugininto a dedicated virtualmodule: virtual:/@storybook/builder-vite/project-annotations.js. This allows tests and external tools to access composed annotations. The project annotations will be re-used in@storybook/addon-vitestin SB 11, since it is considered a breaking change because it might conflict with the user's existing.storybook/vitest.setup.tsfile.4. Cleanups and Optimization
Impact on @storybook/addon-vitest
By leveraging the new
viteCorePluginspreset, addon-vitest can now automatically stay in sync with the builder's configuration for resolve conditions, globals, and docgen, reducing the maintenance burden and preventing configuration drift.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Manual testing should cover:
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-33819-sha-40177d0f. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33819-sha-40177d0f sandboxor in an existing project withnpx storybook@0.0.0-pr-33819-sha-40177d0f upgrade.More information
0.0.0-pr-33819-sha-40177d0fvalentin/encapsulate-vite-plugins40177d0f1771509401)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=33819Summary by CodeRabbit
New Features
Refactor
Chores