Addon Vitest: Support simple vite.config without defineConfig helper#33694
Conversation
|
View your CI Pipeline Execution ↗ for commit d9eaac6
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThis PR extends Vitest configuration validation by generalizing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/cli/AddonVitestService.ts (1)
326-335: Handle plain-object default exports and avoid masking unsafe mergeConfig args.Right now
export default { ... }still falls through as invalid, andsome(...)can accept configs where one mergeConfig object has an unsafetest.workspacebecause another object is safe. Consider (a) explicitly validating object-literal exports, and (b) validating all object-like mergeConfig args while ignoring non-object args.🛠️ Proposed fix
- if (this.isDefineConfigExpression(path.node.declaration)) { - isValidVitestConfig = this.isSafeToExtendWorkspace(path.node.declaration); - } else if (this.isMergeConfigExpression(path.node.declaration)) { - // the config could be anywhere in the mergeConfig call, so we need to check each argument - const mergeCall = path.node.declaration as CallExpression; - isValidVitestConfig = mergeCall.arguments.some((arg) => - this.isSafeToExtendWorkspace(arg as CallExpression) - ); - } + if (this.isDefineConfigExpression(path.node.declaration)) { + isValidVitestConfig = this.isSafeToExtendWorkspace(path.node.declaration); + } else if (babel.types.isObjectExpression(path.node.declaration)) { + isValidVitestConfig = this.isSafeToExtendWorkspace(path.node.declaration); + } else if (this.isMergeConfigExpression(path.node.declaration)) { + // the config could be anywhere in the mergeConfig call, so we need to check each argument + const mergeCall = path.node.declaration as CallExpression; + const objectArgs = mergeCall.arguments.filter( + (arg) => babel.types.isObjectExpression(arg) || babel.types.isCallExpression(arg) + ); + isValidVitestConfig = + objectArgs.length > 0 && objectArgs.every((arg) => this.isSafeToExtendWorkspace(arg)); + }
🧹 Nitpick comments (1)
code/core/src/cli/AddonVitestService.test.ts (1)
605-646: Add coverage for plain-object exports and unsafe mixed mergeConfig args.These new cases are great for the mergeConfig shapes; after supporting object-literal default exports, please add:
- a positive case for
export default { test: { ... } }, and- a negative case where one mergeConfig object has
test.workspaceas a non-array while another object is safe.
This will prevent false positives and lock in the intended safety behavior.
…ort-simple-workspace-config Addon Vitest: Support simple vite.config without defineConfig helper (cherry picked from commit 751f18f)
Closes #33409
What I did
This PR fixes a bug in the Vitest addon configuration validation where mergeConfig calls using plain object literals were incorrectly flagged as incompatible.
Before (broken):
Before (worked):
Both configurations are semantically equivalent and should be supported.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
vitest.config.tsusingmergeConfigwith a plain object literal:npx storybook@next init # or add the addon to existing Storybook npx storybook@next add @storybook/addon-testVerify that:
npx vitest --project=storybookworks correctlyTest with
defineConfigvariant (should continue to work):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-33694-sha-d9eaac60. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33694-sha-d9eaac60 sandboxor in an existing project withnpx storybook@0.0.0-pr-33694-sha-d9eaac60 upgrade.More information
0.0.0-pr-33694-sha-d9eaac60valentin/addon-vitest-support-simple-workspace-configd9eaac601769612114)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=33694Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.