-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vitest: Implement add command for vitest addon #28920
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
storybookTest(),${vitestInfo.frameworkPluginCall ? '\n' + vitestInfo.frameworkPluginCall : ''} | ||
], | ||
test: { | ||
include: ['**/*.{stories}.?(m)[jt]s?(x)'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The glob pattern for stories files might be too permissive
const builderPackageJson = await fs.readFile( | ||
`${typeof builder === 'string' ? builder : builder.name}/package.json`, | ||
'utf8' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Wrap this in a try/catch to handle potential file read errors
☁️ Nx Cloud ReportCI is running/has finished running commands for commit dc9432a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
storybookTest(),${vitestInfo.frameworkPluginCall ? '\n' + vitestInfo.frameworkPluginCall : ''} | ||
], | ||
test: { | ||
include: ['**/*.stories.?(m)[jt]s?(x)'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The glob pattern for stories files might be too permissive
const builderPackageJson = await fs.readFile( | ||
`${typeof builder === 'string' ? builder : builder.name}/package.json`, | ||
'utf8' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Wrap this in a try/catch to handle potential file read errors
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/10473560827 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!! We should revamp all of these messages, hopefully Kyle can assist. It's via the CLI where we will introduce the feature to people, so we should make sure they have a great experience through it.
Here are some suggestions for messages:
- The Vitest addon is going to set up its plugin for you
- Configuring Vitest, Playwright and Vitest browser mode dependencies
- We detected that you're using Next.js. We will configure the
vite-plugin-storybook-nextjs
plugin to allow you to run tests in Vitest. More info at: [some-link-here] - We detected that you have Vitest and workspaces configured, so we couldn't automatically set the plugin for you. Please refer to the documentation on how to complete the set up: [some-link]
- There were some issues when setting up the plugin, please follow the steps to manually set up the plugin instead: [some-link]
- The Vitest addon is now configured and you're ready to run your tests! Check the documentation for more information about its features and configurations at [some-link]
Co-authored-by: Yann Braga <yannbf@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to judge the functional correctness of this, but code looks alright to me.
} | ||
logger.info(c.bold('Installing packages...')); | ||
logger.info(packages.join(', ')); | ||
await packageManager.addDependencies({ installAsDevDependencies: true }, packages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this or any of the other operations fail? Should we wrap in a try/catch to provide some narrative about the failure? Perhaps we may want to add telemetry to track these?
` | ||
); | ||
|
||
const configFiles = extensions.map((ext) => 'vitest.config' + ext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should do this mapping where extensions
are defined as well?
logger.info(c.bold('Writing vitest.config.ts file...')); | ||
await writeFile( | ||
resolve('vitest.config.ts'), | ||
dedent` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these code snippets should probably be in a separate file somewhere, rather than inline.
validateFrameworkName(frameworkName); | ||
const frameworkPackageName = extractProperFrameworkName(frameworkName); | ||
|
||
console.log(configDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be removed.
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/10526145473 |
Closes #28807
What I did
Implement add command for vitest addon
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts
Make 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-28920-sha-cd8f1f27
. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-28920-sha-cd8f1f27 sandbox
or in an existing project withnpx storybook@0.0.0-pr-28920-sha-cd8f1f27 upgrade
.More information
0.0.0-pr-28920-sha-cd8f1f27
kasper/vitest-add
cd8f1f27
1724419799
)To request a new release of this pull request, mention the
@storybookjs/core
team.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=28920
Greptile Summary
This PR enhances the Vitest addon installation process for Storybook, focusing on improved framework support and configuration setup.
code/addons/vitest/src/postinstall.ts
for automatic Vitest setup across various frameworksvitest.workspace.ts
orvitest.config.ts
based on existing project configurationcode/lib/cli-storybook/src/add.ts
to includeconfigDir
in PostinstallOptions for more flexible addon installation