Revamp Vite dependency optimization: use preview annotations as entry points#33875
Revamp Vite dependency optimization: use preview annotations as entry points#33875
Conversation
|
|
View your CI Pipeline Execution ↗ for commit c92f3f4
☁️ Nx Cloud last updated this comment at |
|
Failed to publish canary version of this pull request, triggered by @yannbf. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/22188806376 |
|
@copilot this mostly worked but there are parts which you didn't catch. When running Storybook and watching its output you can see an optimization message for How can we avoid missing these, and are there perhaps more entries which are required here? |
yannbf
changed the title
yannbf
changed the title
These two packages were missed because they appear in contexts Vite's dep scanner can't statically analyze:
The systematic rule to catch these: check virtual module code generators ( After auditing all virtual module generators and dynamic imports, these two are the only cases that need explicit The fixes from commit |
|
Great @copilot but how do we make sure we don't regress? Is there a way to automatically catch cases which are not covered? Can you write a test that would have caught this? |
Added two regression tests in commit 1. Systematic test in Generates all virtual module code ( 2. Focused regression test in Asserts This covers the two identified patterns that Vite cannot discover statically: imports in virtual module string templates, and |
valentinpalkovic
added
ci:daily
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
…ove INCLUDE_CANDIDATES Co-authored-by: yannbf <1671563+yannbf@users.noreply.github.com>
…timizeDeps coverage Co-authored-by: yannbf <1671563+yannbf@users.noreply.github.com>
valentinpalkovic
force-pushed
the
copilot/revamp-vite-vitest-optimization-again
branch
from
9e00f92 to
e96bd64
Compare
…ndency optimization
📝 WalkthroughWalkthroughThis pull request refactors how Vite dependency optimization is configured across Storybook. The approach shifts from a dynamic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🧹 Nitpick comments (2)
code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.ts (2)
102-116: Consider moving mock implementations to abeforeEachblock.The
mockReturnValueOncecalls on lines 103 and 111 are inline mock implementations within test cases. Per coding guidelines, mock behaviors should be implemented inbeforeEachblocks for consistency and to separate setup from assertion logic.Alternative pattern
One approach is to group tests that need specific mock behaviors:
describe('when user preview config exists', () => { beforeEach(() => { loadPreviewOrConfigFileMock.mockReturnValue('/project/.storybook/preview.ts' as any); }); it('adds the user preview config file as an entry', async () => { const plugin = storybookOptimizeDepsPlugin(makeOptions()); const result = await (plugin.config as Function)({}, { command: 'serve' }); expect(result.optimizeDeps.entries).toContain('/project/.storybook/preview.ts'); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.ts` around lines 102 - 116, Move the inline mockReturnValueOnce calls into beforeEach setup blocks to separate setup from assertions: for the tests around storybookOptimizeDepsPlugin/config, create a describe block for the "user preview exists" case and a describe block for the "no user preview" case, and in each describe's beforeEach call loadPreviewOrConfigFileMock.mockReturnValue(...) (using the concrete '/project/.storybook/preview.ts' value for the first and undefined for the second) before invoking storybookOptimizeDepsPlugin(makeOptions()) in the its; this keeps tests focused and ensures loadPreviewOrConfigFileMock is consistently configured before each test.
7-12: Addspy: trueoption tovi.mock()call.As per coding guidelines, all
vi.mock()calls should use thespy: trueoption for consistent mocking patterns.Proposed fix
-vi.mock('storybook/internal/common', () => ({ - loadPreviewOrConfigFile: vi.fn(() => undefined), -})); +vi.mock('storybook/internal/common', { spy: true });Then set up the default mock behavior in a
beforeEachblock:beforeEach(() => { vi.mocked(loadPreviewOrConfigFile).mockReturnValue(undefined); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.ts` around lines 7 - 12, Update the vi.mock call to include the spy: true option (vi.mock('storybook/internal/common', { spy: true }, ...)) and remove the inline mock-return in the factory; instead, in a beforeEach block call vi.mocked(loadPreviewOrConfigFile).mockReturnValue(undefined) to set the default behavior; reference the existing loadPreviewOrConfigFile import and any loadPreviewOrConfigFileMock usage to ensure tests use the spied mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/tasks/sandbox-parts.ts`:
- Around line 533-539: The current replacement for linked mode uses a separate
fileContent.replace that re-matches the same plugins block and inadvertently
removes resolve.preserveSymlinks; update the logic that handles options.link
(the fileContent.replace call that matches /(plugins\s*:\s*\[[^\]]*\],?)/) to
perform a single replacement that inserts both the server.fs.allow block and
preserves/ensures resolve.preserveSymlinks inside the resolve section (i.e.,
include resolve: { preserveSymlinks: true } alongside the server: { fs: { allow:
['../../..'] } } in the replacement) so the earlier preserveSymlinks setting is
not overwritten when options.link is true.
---
Nitpick comments:
In
`@code/builders/builder-vite/src/plugins/storybook-optimize-deps-plugin.test.ts`:
- Around line 102-116: Move the inline mockReturnValueOnce calls into beforeEach
setup blocks to separate setup from assertions: for the tests around
storybookOptimizeDepsPlugin/config, create a describe block for the "user
preview exists" case and a describe block for the "no user preview" case, and in
each describe's beforeEach call loadPreviewOrConfigFileMock.mockReturnValue(...)
(using the concrete '/project/.storybook/preview.ts' value for the first and
undefined for the second) before invoking
storybookOptimizeDepsPlugin(makeOptions()) in the its; this keeps tests focused
and ensures loadPreviewOrConfigFileMock is consistently configured before each
test.
- Around line 7-12: Update the vi.mock call to include the spy: true option
(vi.mock('storybook/internal/common', { spy: true }, ...)) and remove the inline
mock-return in the factory; instead, in a beforeEach block call
vi.mocked(loadPreviewOrConfigFile).mockReturnValue(undefined) to set the default
behavior; reference the existing loadPreviewOrConfigFile import and any
loadPreviewOrConfigFileMock usage to ensure tests use the spied mock.
| // In linked mode, add server.fs.allow to allow Vite to serve files from the monorepo root | ||
| if (options.link) { | ||
| fileContent = fileContent.replace(/(plugins\s*:\s*\[[^\]]*\],?)/, (match) => { | ||
| // Insert server.fs.allow after plugins (alongside resolve) | ||
| return `${match}\n server: {\n fs: {\n allow: ['../../..']\n }\n },`; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix linked-mode replace: it drops preserveSymlinks.
On Line 533 the second replace re-matches the original plugins block and overwrites the earlier insertion, so resolve.preserveSymlinks is removed whenever options.link is true. This can reintroduce symlink resolution issues in linked sandboxes.
🔧 Suggested fix (single replace that inserts both blocks)
- fileContent = fileContent.replace(/(plugins\s*:\s*\[[^\]]*\],?)/, (match) => {
- // Insert resolve after plugins
- return `${match}\n resolve: {\n preserveSymlinks: true\n },`;
- });
-
- // In linked mode, add server.fs.allow to allow Vite to serve files from the monorepo root
- if (options.link) {
- fileContent = fileContent.replace(/(plugins\s*:\s*\[[^\]]*\],?)/, (match) => {
- // Insert server.fs.allow after plugins (alongside resolve)
- return `${match}\n server: {\n fs: {\n allow: ['../../..']\n }\n },`;
- });
- }
+ fileContent = fileContent.replace(/(plugins\s*:\s*\[[^\]]*\],?)/, (match) => {
+ const resolveBlock = ` resolve: {\n preserveSymlinks: true\n },`;
+ const serverBlock = options.link
+ ? `\n server: {\n fs: {\n allow: ['../../..']\n }\n },`
+ : '';
+ return `${match}\n${resolveBlock}${serverBlock}`;
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tasks/sandbox-parts.ts` around lines 533 - 539, The current
replacement for linked mode uses a separate fileContent.replace that re-matches
the same plugins block and inadvertently removes resolve.preserveSymlinks;
update the logic that handles options.link (the fileContent.replace call that
matches /(plugins\s*:\s*\[[^\]]*\],?)/) to perform a single replacement that
inserts both the server.fs.allow block and preserves/ensures
resolve.preserveSymlinks inside the resolve section (i.e., include resolve: {
preserveSymlinks: true } alongside the server: { fs: { allow: ['../../..'] } }
in the replacement) so the earlier preserveSymlinks setting is not overwritten
when options.link is true.
Vite's dep optimizer was only scanning story files as entry points, missing preview annotation files (user's
preview.ts, addon/framework/renderer previews). This caused runtime CJS dependency discovery, triggering hard reloads in Storybook and flaky test reruns in Vitest.The real fix is to give Vite the complete set of entry points upfront so it can crawl all transitive dependencies before serving — no hard reloads, no Vitest instability.
Changes
storybook-optimize-deps-plugin.ts: Fetches allpreviewAnnotationsfrom presets (in parallel) and adds the user's preview file + all addon/framework/renderer annotation files tooptimizeDeps.entries. Vite crawls these to auto-discover CJS deps.constants.ts(deleted): Removes the ~100-entryINCLUDE_CANDIDATEShardcoded list — no longer needed.optimizeDeps.ts(deleted): Removes the outdatedgetOptimizeDeps()that usedresolveConfig+asyncFilterto filterINCLUDE_CANDIDATES.vite-server.ts: RemovesgetOptimizeDepscall and the redundant include-merging logic.builder-vite/src/preset.ts: AddsoptimizeViteDepsexport forstorybook/internal/preview/runtime, which is only referenced in the virtual iframe entry module and cannot be discovered by Vite's static dep scanner.renderers/react/src/preset.ts: AddsoptimizeViteDepsexport forreact-dom/test-utils, which is dynamically imported and not statically analyzable by Vite.storybook-optimize-deps-plugin.test.ts(new): Tests covering entries deduplication, preview annotation entries, string/array config merging, and preset include passthrough.codegen-modern-iframe-script.test.ts: Adds a systematic regression test that generates all virtual module code, extracts every non-virtual package import, and asserts each one is either inoptimizeViteDepsor documented as discovered via entry crawling. This will automatically catch future cases where a package is added to a virtual module code generator without updatingoptimizeViteDeps.renderers/react/src/preset.test.ts(new): Focused regression test assertingreact-dom/test-utilsis inoptimizeViteDeps, with a comment explaining it is only accessible via a dynamicawait import()inact-compat.ts.The
optimizeViteDepspreset mechanism is preserved for frameworks that need explicit includes (nextjs-vite, sveltekit, addon-docs). The fix applies to both@storybook/builder-viteand@storybook/addon-vitestsincestorybookOptimizeDepsPluginis part of the sharedviteCorePlugins.Why explicit
includeis still needed for some packagesMost
storybook/internal/*packages have acodeexport condition resolving to TypeScript source, so Vite treats them as ESM and discovers them naturally via entry-point crawling. Two packages are exceptions:storybook/internal/preview/runtime: Only imported inside a string template incodegen-modern-iframe-script.ts(the virtual iframe entry). Vite's dep scanner never sees virtual module contents and discovers this dependency only at request time.react-dom/test-utils: Dynamically imported viaawait import()inact-compat.ts, which Vite's static analyzer does not pre-bundle.Original prompt
This section details on the original issue you should resolve
<issue_title>[Bug]: Revamp Vite and Vitest dependency optimization</issue_title>
<issue_description>### Describe the bug
Problem
We currently have specific code which handles dependency optimization in the Vite builder as can be seen here. That code is outdated and might not be necessary.
When the Vite dependency optimization kicks in, it needs to hard reload the browser. This causes two things:
Storybook already adds stories as
entriesto Vite's config, which makes it scan most files to do dependency optimization automatically (great). However, currently Storybook does not add other necessary files as entries, such as the preview annotation files. For instance, if a.storybook/preview.tsxfile has imports like:Then such dependencies might need to be optimized (if they're CJS for instance) and therefore this will trigger the warnings. This would not have been the case had Storybook added the preview file as an entry to Vite. The same issue also occurs from packages, such as renderers, frameworks or addons. Such packages can use a concept of a
preview annotationorpreset, which adds annotations to Storybook (such as decorators, loaders, etc). These annotations can also end up making imports of dependencies that would cause the same problem.This is a quite critical change that requires a lot of checking, but worth investigating. This solution will close many issues, including #33067, #32049, #31119 as well as provide a much better experience for end users.
The workaround
When this behavior occurs, users see, both in Storybook's CLI and in Vitest's CLI some warnings like this:
So to solve this issue, users need to add a
viteFinalconfig in the Storybook main file and specify their own dependencies in itsoptimizeDeps.includefield:Potential solution
Vite automatically handles dependency optimization based on the entrypoints passed into the
entries, so theoretically we don't need to handle dependency optimization ourselves, as long as we specify the correct entries.Vitest has similar challenges and solves it by doing this, it basically adds all test files + setup files (plus some internal utilities) as entries, and Vite will figure out what needs to be optimized. In Storybook, the scenario is similar, we need to list the stories files as entries, as well as the preview entries (user's preview file, plus addon/renderer/frameworks preview annotations which come from presets or preview files).
</issue_description>
<agent_instructions>There was a recent change in the repo to centralize core vite plugins and that is reused between storybook proper and the Vitest integration. The solution you come up with should work for them both then.
Also, this issue is not necessarily a problem for CSF4 (known as csf factories) users, it's mostly a problem for CSF3 and below.
The main goal of this task should be to improve and simplify the dependency optimization logic, which will make the INCLUDE_CANDIDATES in code/...
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
Release Notes
New Features
Chores