Builder-Vite: Support configLoader via builder options#34080
Builder-Vite: Support configLoader via builder options#34080holvi-sebastian wants to merge 5 commits into
Conversation
|
View your CI Pipeline Execution ↗ for commit 9d88c25
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Storybook CLI
participant Builder as Builder-Vite
participant Vite as Vite (loadConfigFromFile)
participant Preset as Vitest Preset
participant Manager as Vitest Manager / TestRunner
CLI->>Builder: start build/serve (options include configLoader)
Builder->>Vite: loadConfigFromFile(..., configLoader)
Note over Builder,Vite: Vite resolves user config using specified loader
CLI->>Preset: initialize addons (reads core.builder.options)
Preset->>Manager: runTestRunner(..., configLoader)
Manager->>Manager: set env STORYBOOK_CONFIG_LOADER=configLoader
Manager->>Manager: spawn child test-runner process (reads env)
Manager->>Manager: start Vitest with createVitest({ configLoader })
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (3)
code/builders/builder-vite/src/vite-config.test.ts (2)
11-15: Consider usingspy: trueoption for the Vite mock.Per coding guidelines,
vi.mock()should use thespy: trueoption for all package mocks. This applies to the existing mock setup as well.♻️ Suggested fix
-vi.mock('vite', async (importOriginal) => ({ +vi.mock('vite', { spy: true }, async (importOriginal) => ({ ...(await importOriginal<typeof import('vite')>()), loadConfigFromFile: vi.fn(async () => ({})), defaultClientConditions: undefined, }));As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/vite-config.test.ts` around lines 11 - 15, The test's module mock for 'vite' uses vi.mock(...) without the required spy option; update the vi.mock call for 'vite' to include the spy: true option so the mock follows the repository guideline (i.e., call vi.mock('vite', async (importOriginal) => ({ ... }), { spy: true });) while keeping the existing overrides for loadConfigFromFile and defaultClientConditions intact; ensure the vi.mock invocation that references loadConfigFromFile and defaultClientConditions is the one updated.
113-119: Move mock implementation tobeforeEachblock per coding guidelines.The mock behavior is implemented inline within the test case. Per coding guidelines, mock implementations should be placed in
beforeEachblocks to maintain consistency across tests.However, since this test requires a specific mock return value different from other tests, the current approach with
mockReturnValueOnceis acceptable for this isolated case. Consider documenting why this inline mock is necessary.As per coding guidelines: "Implement mock behaviors in
beforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/vite-config.test.ts` around lines 113 - 119, Move the inline mockReturnValueOnce call for loadConfigFromFileMock into the test suite's beforeEach to follow the Vitest guideline (or, if this test truly requires a unique return value, add a short comment above the inline mock explaining why it must remain inline); locate the mock usage on loadConfigFromFileMock in vite-config.test.ts and either initialize the same Promise.resolve({ config: {}, path: '', dependencies: [] }) in the beforeEach setup or add a one-line justification comment next to the mockReturnValueOnce so reviewers understand this exception.code/builders/builder-vite/src/types.ts (1)
24-24: Missing trailing semicolon for consistency.Line 23 (
viteConfigPath) has a semicolon, but line 24 (configLoader) does not. Add a semicolon for consistency with the rest of the codebase.✏️ Suggested fix
- configLoader?: 'bundle' | 'runner' | 'native' + configLoader?: 'bundle' | 'runner' | 'native';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-vite/src/types.ts` at line 24, The property declaration for configLoader in types.ts is missing a trailing semicolon; update the configLoader?: 'bundle' | 'runner' | 'native' declaration to include a trailing semicolon to match the style of viteConfigPath and the rest of the file (i.e., add ";" at the end of the configLoader line).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/builders/builder-vite/src/vite-config.test.ts`:
- Around line 92-131: The test that asserts configLoader is passed to
loadConfigFromFile is declared at top-level and should be moved inside the
existing describe('commonConfig') block to keep tests organized and use the same
setup/teardown; relocate the entire it('should pass configLoader option to
loadConfigFromFile', ...) block so it sits within the describe('commonConfig')
scope that contains the other commonConfig tests, ensuring it still references
commonConfig and loadConfigFromFileMock as before.
---
Nitpick comments:
In `@code/builders/builder-vite/src/types.ts`:
- Line 24: The property declaration for configLoader in types.ts is missing a
trailing semicolon; update the configLoader?: 'bundle' | 'runner' | 'native'
declaration to include a trailing semicolon to match the style of viteConfigPath
and the rest of the file (i.e., add ";" at the end of the configLoader line).
In `@code/builders/builder-vite/src/vite-config.test.ts`:
- Around line 11-15: The test's module mock for 'vite' uses vi.mock(...) without
the required spy option; update the vi.mock call for 'vite' to include the spy:
true option so the mock follows the repository guideline (i.e., call
vi.mock('vite', async (importOriginal) => ({ ... }), { spy: true });) while
keeping the existing overrides for loadConfigFromFile and
defaultClientConditions intact; ensure the vi.mock invocation that references
loadConfigFromFile and defaultClientConditions is the one updated.
- Around line 113-119: Move the inline mockReturnValueOnce call for
loadConfigFromFileMock into the test suite's beforeEach to follow the Vitest
guideline (or, if this test truly requires a unique return value, add a short
comment above the inline mock explaining why it must remain inline); locate the
mock usage on loadConfigFromFileMock in vite-config.test.ts and either
initialize the same Promise.resolve({ config: {}, path: '', dependencies: [] })
in the beforeEach setup or add a one-line justification comment next to the
mockReturnValueOnce so reviewers understand this exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73ec27cc-b218-423c-b8fe-03161a4c5cc7
📒 Files selected for processing (5)
code/builders/builder-vite/src/types.tscode/builders/builder-vite/src/vite-config.test.tscode/builders/builder-vite/src/vite-config.tsdocs/_snippets/main-config-builder-configLoader.mddocs/builders/vite.mdx
| it('should pass configLoader option to loadConfigFromFile', async () => { | ||
| const optionsWithConfigLoader: Options = { | ||
| ...dummyOptions, | ||
| presets: { | ||
| apply: async (key: string) => | ||
| ({ | ||
| framework: { name: '' }, | ||
| addons: [], | ||
| core: { | ||
| builder: { | ||
| name: '@storybook/builder-vite', | ||
| options: { | ||
| configLoader: 'native' | ||
| } | ||
| }, | ||
| }, | ||
| options: {}, | ||
| })[key], | ||
| } as Presets, | ||
| }; | ||
|
|
||
| loadConfigFromFileMock.mockReturnValueOnce( | ||
| Promise.resolve({ | ||
| config: {}, | ||
| path: '', | ||
| dependencies: [], | ||
| }) | ||
| ); | ||
|
|
||
| await commonConfig(optionsWithConfigLoader, 'development'); | ||
|
|
||
| // Verify loadConfigFromFile was called with configLoader as the 6th argument | ||
| expect(loadConfigFromFileMock).toHaveBeenCalledWith( | ||
| expect.objectContaining({ command: 'serve' }), | ||
| undefined, | ||
| expect.any(String), | ||
| undefined, | ||
| undefined, | ||
| 'native' | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test should be inside a describe block for proper organization.
This test is defined at the top level (outside the describe('commonConfig') block ending at line 52). Since this test exercises commonConfig, it should be placed inside that describe block for consistency and proper test organization.
♻️ Suggested fix
Move the test inside the describe('commonConfig') block:
describe('commonConfig', () => {
it('should set configFile to false and include plugins', async () => {
// ... existing test
});
+
+ it('should pass configLoader option to loadConfigFromFile', async () => {
+ // ... test implementation
+ });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/builders/builder-vite/src/vite-config.test.ts` around lines 92 - 131,
The test that asserts configLoader is passed to loadConfigFromFile is declared
at top-level and should be moved inside the existing describe('commonConfig')
block to keep tests organized and use the same setup/teardown; relocate the
entire it('should pass configLoader option to loadConfigFromFile', ...) block so
it sits within the describe('commonConfig') scope that contains the other
commonConfig tests, ensuring it still references commonConfig and
loadConfigFromFileMock as before.
|
Thank you for the PR. Vite 8 will no longer use |
|
As of I think the future plan is to default to |
My understanding is that there may have been a chance your specific issue was resolved, because in Vite 8, not esbuild, but rolldown is used to bundle the config into a temporary file and load it, and that rolldown may behave differently or be more advanced with TypeScript handling in config files. I'll take a look at your PR. I think this change doesn't cover |
valentinpalkovic
changed the title
|
Your reproduction is incomplete and hard to follow. I've created a reproduction here: https://github.com/valentinpalkovic/config-loader-repo But I actually can't reproduce the issue. Can you please take a look, create a fork, and change the reproduction so it reproduces the issue? Thanks! |
valentinpalkovic
added
the
needs reproduction
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/addons/vitest/src/node/test-manager.ts`:
- Around line 64-65: The configLoader property is declared private in
TestManager but VitestManager accesses this.testManager.configLoader; change the
declaration of configLoader in test-manager.ts from private to public readonly
so it is accessible but immutable (i.e., update the member signature for
configLoader: TestManagerOptions['configLoader'] to be public readonly). Ensure
any constructor/assignment still assigns the value and update any references
assuming mutability if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 145d94d3-780f-4d69-9f4b-35456022c45b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
code/addons/vitest/package.jsoncode/addons/vitest/src/node/boot-test-runner.tscode/addons/vitest/src/node/test-manager.tscode/addons/vitest/src/node/vitest-manager.tscode/addons/vitest/src/node/vitest.tscode/addons/vitest/src/preset.ts
| private configLoader?: TestManagerOptions['configLoader']; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Declaration in TestManager:"
rg -n -C2 'configLoader' code/addons/vitest/src/node/test-manager.ts
echo
echo "External access from VitestManager:"
rg -n -C2 'testManager\.configLoader' code/addons/vitest/src/node/vitest-manager.tsRepository: storybookjs/storybook
Length of output: 970
configLoader cannot be private if VitestManager reads it.
Line 127 in vitest-manager.ts accesses this.testManager.configLoader, but the member is declared private here. This violates TypeScript strict mode. Change it to public readonly.
Fix
- private configLoader?: TestManagerOptions['configLoader'];
+ public readonly configLoader?: TestManagerOptions['configLoader'];📝 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.
| private configLoader?: TestManagerOptions['configLoader']; | |
| public readonly configLoader?: TestManagerOptions['configLoader']; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/addons/vitest/src/node/test-manager.ts` around lines 64 - 65, The
configLoader property is declared private in TestManager but VitestManager
accesses this.testManager.configLoader; change the declaration of configLoader
in test-manager.ts from private to public readonly so it is accessible but
immutable (i.e., update the member signature for configLoader:
TestManagerOptions['configLoader'] to be public readonly). Ensure any
constructor/assignment still assigns the value and update any references
assuming mutability if needed.
|
Done, at: valentinpalkovic/config-loader-repo#1 |
valentinpalkovic
removed
the
needs reproduction
|
I want to get a second opinion from the team regarding this feature request: |
|
I've discussed it internally, and we want to proceed with this fix. I'll try to get CI green and then build a canary release to test the fix! |
|
Fix verified in valentinpalkovic/config-loader-repo#1. Though, addon-vitest crashes. I'll take a further look! |
There was a problem hiding this comment.
Actionable comments posted: 1
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/node/boot-test-runner.ts (1)
87-94:⚠️ Potential issue | 🟡 MinorConditionally set
STORYBOOK_CONFIG_LOADERonly when defined.When
configLoaderisundefined, omitSTORYBOOK_CONFIG_LOADERfrom the spawned child's env object entirely. According to Node.js documentation, passingundefinedvalues in theenvoption tospawncauses them to be ignored (not set), but explicitly omitting the key when undefined is clearer and avoids reliance on this behavior.The downstream code in
code/addons/vitest/src/node/vitest.ts(line 53) casts the env value directly:configLoader: process.env.STORYBOOK_CONFIG_LOADER as BuilderOptions['configLoader']. If not set, it will beundefined, which is the correct behavior.Apply the fix
env: { VITEST: 'true', TEST: 'true', VITEST_CHILD_PROCESS: 'true', NODE_ENV: process.env.NODE_ENV ?? 'test', STORYBOOK_CONFIG_DIR: normalize(options.configDir), - STORYBOOK_CONFIG_LOADER: configLoader, + ...(configLoader ? { STORYBOOK_CONFIG_LOADER: configLoader } : {}), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/node/boot-test-runner.ts` around lines 87 - 94, The env object currently always includes STORYBOOK_CONFIG_LOADER with value configLoader; change this so STORYBOOK_CONFIG_LOADER is only added when configLoader is not undefined—e.g., build the env object for the spawn call and conditionally add the key (using an if or a spread like ...(configLoader !== undefined ? { STORYBOOK_CONFIG_LOADER: configLoader } : {})); update the code around where env is constructed in boot-test-runner.ts so VITEST/TEST/etc remain unchanged but STORYBOOK_CONFIG_LOADER is omitted entirely when configLoader is undefined to ensure downstream code in vitest.ts receives undefined when not set.
♻️ Duplicate comments (1)
code/addons/vitest/src/node/test-manager.ts (1)
64-64:⚠️ Potential issue | 🔴 Critical
configLoaderis read externally and cannot remainprivate.
VitestManager.startVitestreadsthis.testManager.configLoader(vitest-manager.ts L127), but this declaration isprivate, which TypeScript will reject. Switch topublic readonly(or expose via a getter).🔧 Fix
- private configLoader?: TestManagerOptions['configLoader']; + public readonly configLoader?: TestManagerOptions['configLoader'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/node/test-manager.ts` at line 64, The field configLoader on TestManager is declared private but is accessed externally (VitestManager.startVitest reads this.testManager.configLoader), so change its visibility to a public readonly field or add a public getter; update the declaration "private configLoader?: TestManagerOptions['configLoader'];" to "public readonly configLoader?: TestManagerOptions['configLoader'];" (or implement a public get configLoader(): TestManagerOptions['configLoader'] { return this._configLoader; }) and ensure any internal assignments use the backing field if you choose the getter approach.
🧹 Nitpick comments (1)
code/addons/vitest/src/preset.ts (1)
87-89: Optional: simplify theconfigLoaderderivation.The current expression yields a
false | 'bundle' | 'runner' | 'native' | undefinedunion (because of the&&), which is why both call sites needconfigLoader || undefinedto launder thefalse. A direct ternary or optional chaining is clearer and avoids the type assertion.♻️ Suggested simplification
- const configLoader = - typeof core.builder !== 'string' && - (core.builder?.options?.configLoader as BuilderOptions['configLoader']); + const configLoader: BuilderOptions['configLoader'] | undefined = + typeof core.builder === 'string' ? undefined : core.builder?.options?.configLoader;…and then drop
|| undefinedat the tworunTestRunner({...})call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/preset.ts` around lines 87 - 89, The expression assigning configLoader currently uses && which produces a false | ... union; change it to a clear conditional so configLoader is either the BuilderOptions['configLoader'] value or undefined (for example use a ternary: check typeof core.builder !== 'string' ? core.builder?.options?.configLoader : undefined or an equivalent optional-chaining conditional) and then remove the now-unnecessary `|| undefined` at the runTestRunner(...) call sites; update references to the variable name configLoader and the call sites that pass it to runTestRunner so they pass the value directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/builders/builder-vite/src/vite-config.ts`:
- Around line 45-53: Detect when the user provided BuilderOptions.configLoader
and the installed Vite version is older than 6.1.0, and surface a clear runtime
error; inside vite-config.ts (where getBuilderOptions and loadConfigFromFile are
used and the local variable configLoader is passed into loadConfigFromFile),
read the installed Vite version (e.g. require('vite/package.json').version or
import) and use semver comparison (require('semver').lt or similar) to check if
version < 6.1.0; if so, throw or processLogger.error+process.exit(1) with a
short message explaining that configLoader (including 'native') requires Vite
>=6.1.0 and instruct the user to upgrade Vite or remove configLoader, otherwise
proceed to call loadConfigFromFile as currently implemented.
---
Outside diff comments:
In `@code/addons/vitest/src/node/boot-test-runner.ts`:
- Around line 87-94: The env object currently always includes
STORYBOOK_CONFIG_LOADER with value configLoader; change this so
STORYBOOK_CONFIG_LOADER is only added when configLoader is not undefined—e.g.,
build the env object for the spawn call and conditionally add the key (using an
if or a spread like ...(configLoader !== undefined ? { STORYBOOK_CONFIG_LOADER:
configLoader } : {})); update the code around where env is constructed in
boot-test-runner.ts so VITEST/TEST/etc remain unchanged but
STORYBOOK_CONFIG_LOADER is omitted entirely when configLoader is undefined to
ensure downstream code in vitest.ts receives undefined when not set.
---
Duplicate comments:
In `@code/addons/vitest/src/node/test-manager.ts`:
- Line 64: The field configLoader on TestManager is declared private but is
accessed externally (VitestManager.startVitest reads
this.testManager.configLoader), so change its visibility to a public readonly
field or add a public getter; update the declaration "private configLoader?:
TestManagerOptions['configLoader'];" to "public readonly configLoader?:
TestManagerOptions['configLoader'];" (or implement a public get configLoader():
TestManagerOptions['configLoader'] { return this._configLoader; }) and ensure
any internal assignments use the backing field if you choose the getter
approach.
---
Nitpick comments:
In `@code/addons/vitest/src/preset.ts`:
- Around line 87-89: The expression assigning configLoader currently uses &&
which produces a false | ... union; change it to a clear conditional so
configLoader is either the BuilderOptions['configLoader'] value or undefined
(for example use a ternary: check typeof core.builder !== 'string' ?
core.builder?.options?.configLoader : undefined or an equivalent
optional-chaining conditional) and then remove the now-unnecessary `||
undefined` at the runTestRunner(...) call sites; update references to the
variable name configLoader and the call sites that pass it to runTestRunner so
they pass the value directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74789f56-89e0-4f45-a789-5f3c3a415085
📒 Files selected for processing (8)
code/addons/vitest/package.jsoncode/addons/vitest/src/node/boot-test-runner.tscode/addons/vitest/src/node/test-manager.tscode/addons/vitest/src/node/vitest-manager.tscode/addons/vitest/src/node/vitest.tscode/addons/vitest/src/preset.tscode/builders/builder-vite/src/vite-config.test.tscode/builders/builder-vite/src/vite-config.ts
✅ Files skipped from review due to trivial changes (1)
- code/addons/vitest/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- code/addons/vitest/src/node/vitest.ts
- code/builders/builder-vite/src/vite-config.test.ts
| const { viteConfigPath, configLoader } = await getBuilderOptions<BuilderOptions>(options); | ||
|
|
||
| const projectRoot = resolve(options.configDir, '..'); | ||
|
|
||
| // I destructure away the `build` property from the user's config object | ||
| // I do this because I can contain config that breaks storybook, such as we had in a lit project. | ||
| // If the user needs to configure the `build` they need to do so in the viteFinal function in main.js. | ||
| const { config: { build: buildProperty = undefined, ...userConfig } = {} } = | ||
| (await loadConfigFromFile(configEnv, viteConfigPath, projectRoot)) ?? {}; | ||
| (await loadConfigFromFile(configEnv, viteConfigPath, projectRoot, undefined, undefined, configLoader)) ?? {}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect builder-vite's Vite dependency range and any version checks.
fd -t f 'package.json' code/builders/builder-vite -x cat {}
echo '---'
# Look for any runtime Vite version guards in the package.
rg -nP --type=ts -C2 '\b(vite/package|VITE_VERSION|version)\b' code/builders/builder-viteRepository: storybookjs/storybook
Length of output: 4225
🌐 Web query:
In which Vite version was the configLoaderoption (with values 'bundle' | 'runner' | 'native') added toloadConfigFromFile?
💡 Result:
Vite version 6.1.0
Citations:
- 1: https://github.com/vitejs/vite/blob/v6.1.6/packages/vite/CHANGELOG.md
- 2: https://github.com/vitejs/vite/blob/v6.1.0/packages/vite/CHANGELOG.md
- 3: Stabilizing `--config-loader=runner` / `--config-loader=native` vitejs/vite#20162
- 4: feat: use module runner to import the config vitejs/vite#18637
- 5: feat: use native runtime to import the config vitejs/vite#19178
- 6: vitejs/vite@7c2a794
🏁 Script executed:
# Check for any version guards or conditionals around configLoader in vite-config.ts
rg -n 'configLoader' code/builders/builder-vite/src/vite-config.ts -B3 -A3
# Check the full context of where configLoader is used
cat -n code/builders/builder-vite/src/vite-config.ts | sed -n '40,60p'
# Look for any version checks related to configLoader elsewhere
rg -n 'configLoader' code/builders/builder-vite/src --type tsRepository: storybookjs/storybook
Length of output: 3127
🏁 Script executed:
# Check BuilderOptions type definition for any comments about configLoader version requirements
rg -n -A5 'configLoader' code/builders/builder-vite/src/types.ts
# Check if there's a minimum version requirement documented or enforced in peerDependencies
cat code/builders/builder-vite/package.json | grep -A2 -B2 'peerDependencies'
# Search for any changelog or recent commits mentioning configLoader
git log --oneline --all --grep='configLoader' code/builders/builder-vite 2>/dev/null | head -5 || echo "No git log available"
# Check if there are any version guards elsewhere in the codebase
rg 'version.*6\.1' code/builders/builder-vite --type tsRepository: storybookjs/storybook
Length of output: 255
LGTM — clean pass-through, but version compatibility gap remains unresolved.
The change correctly threads configLoader from builder options into Vite's loadConfigFromFile as the 6th positional argument. When undefined (the default), prior behavior is preserved. The position and typing match the test in vite-config.test.ts and the type definition in types.ts.
However, configLoader (including the 'native' value) was added in Vite 6.1.0. The peer dependency range still permits Vite 5.0.0+, so users on Vite 5.x or 6.0.x who explicitly set configLoader: 'native' will have it silently ignored by Vite with no warning or error.
Either bump the minimum peer dependency to ^6.1.0 || ^7.0.0 || ^8.0.0, add a runtime version check with a helpful error message, or at minimum document in BuilderOptions that configLoader requires Vite 6.1.0+.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/builders/builder-vite/src/vite-config.ts` around lines 45 - 53, Detect
when the user provided BuilderOptions.configLoader and the installed Vite
version is older than 6.1.0, and surface a clear runtime error; inside
vite-config.ts (where getBuilderOptions and loadConfigFromFile are used and the
local variable configLoader is passed into loadConfigFromFile), read the
installed Vite version (e.g. require('vite/package.json').version or import) and
use semver comparison (require('semver').lt or similar) to check if version <
6.1.0; if so, throw or processLogger.error+process.exit(1) with a short message
explaining that configLoader (including 'native') requires Vite >=6.1.0 and
instruct the user to upgrade Vite or remove configLoader, otherwise proceed to
call loadConfigFromFile as currently implemented.
2 participants
This PR allows the
configLoaderoption / command-line arg to be passed tovitebybuilder-vite.This solves a problem that occurs when importing from another package in
vite.config.tsthat exports.tsfiles directly. This is a common scenario in monorepos configured in a 'just-in-time' setup, and has been a broadly requested issue invite: vitejs/vite#5370Closes #
This PR relates to the following feature requests:
#33390
#32886
Oddly, there don't seem to be any issues that relate to this topic, or if there are I haven't found the right keywords to find them.
What I did
This PR adds pass-through support for the
configLoaderparam onvite'sloadConfigFromFilecall incommonConfig, by reading the value from builder options. This is equivalent to passing--configLoader <value>toviteat the command line.Checklist for Contributors
Testing
Didn't see an obvious place where this option could be tested. I looked for an equivalent test for the
viteConfigPathoption, but didn't find it, so I added it tovite-config.test.ts.The changes in this PR are covered in the following automated tests:
Manual testing
yarn task --task sandbox --start-from auto --template react-vite/default-ts.tsfile intovite.config.tsof the template from another workspace project in the monorepo.storybook buildWithout this PR,
storybookwill fail, expecting.jsto exist, but instead finding.ts... it is unable to determine what to do with the `.ts file.With the PR, the
buildcompletes as normal.Documentation
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-34080-sha-088d952e. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34080-sha-088d952e sandboxor in an existing project withnpx storybook@0.0.0-pr-34080-sha-088d952e upgrade.More information
0.0.0-pr-34080-sha-088d952enext088d952e1774010930)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=34080Summary by CodeRabbit
New Features
Documentation
Tests