Skip to content
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

Merged
merged 30 commits into from
Aug 23, 2024
Merged

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Aug 19, 2024

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:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/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 running npx storybook@0.0.0-pr-28920-sha-cd8f1f27 sandbox or in an existing project with npx storybook@0.0.0-pr-28920-sha-cd8f1f27 upgrade.

More information
Published version 0.0.0-pr-28920-sha-cd8f1f27
Triggered by @kasperpeulen
Repository storybookjs/storybook
Branch kasper/vitest-add
Commit cd8f1f27
Datetime Fri Aug 23 13:29:59 UTC 2024 (1724419799)
Workflow run 10526618266

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

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 76.4 MB 76.4 MB 557 B 3.57 0%
initSize 169 MB 169 MB 800 B 3.27 0%
diffSize 92.8 MB 92.8 MB 243 B 0.91 0%
buildSize 7.46 MB 7.46 MB 187 B 1.04 0%
buildSbAddonsSize 1.62 MB 1.62 MB 106 B 0.91 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.3 MB 2.3 MB 0 B - 0%
buildSbPreviewSize 351 kB 352 kB 81 B 0.75 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.46 MB 4.46 MB 187 B 0.85 0%
buildPreviewSize 3 MB 3 MB 0 B 1.43 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 23.8s 23.1s -697ms 0.78 -3%
generateTime 21.4s 18.7s -2s -702ms -1.18 -14.4%
initTime 18.4s 15.8s -2s -633ms -1.24 🔰-16.6%
buildTime 11.3s 12.9s 1.5s 0.24 11.9%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 7.3s 7.1s -249ms -0.44 -3.5%
devManagerResponsive 4.8s 4.6s -183ms -0.54 -3.9%
devManagerHeaderVisible 890ms 950ms 60ms 1.05 6.3%
devManagerIndexVisible 924ms 995ms 71ms 1.11 7.1%
devStoryVisibleUncached 1.3s 1.4s 104ms 0.46 7.3%
devStoryVisible 923ms 993ms 70ms 1.07 7%
devAutodocsVisible 676ms 768ms 92ms 0.18 12%
devMDXVisible 682ms 670ms -12ms -0.41 -1.8%
buildManagerHeaderVisible 860ms 811ms -49ms 0.61 -6%
buildManagerIndexVisible 870ms 819ms -51ms 0.64 -6.2%
buildStoryVisible 912ms 1s 94ms 1.8 🔺9.3%
buildAutodocsVisible 684ms 677ms -7ms -0.35 -1%
buildMDXVisible 629ms 777ms 148ms 1.87 🔺19%

Greptile Summary

This PR enhances the Vitest addon installation process for Storybook, focusing on improved framework support and configuration setup.

  • Implemented postinstall function in code/addons/vitest/src/postinstall.ts for automatic Vitest setup across various frameworks
  • Added framework-specific plugin support for Next.js and SvelteKit in Vitest configuration
  • Introduced automatic generation of vitest.workspace.ts or vitest.config.ts based on existing project configuration
  • Updated code/lib/cli-storybook/src/add.ts to include configDir in PostinstallOptions for more flexible addon installation

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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

code/addons/vitest/src/postinstall.ts Show resolved Hide resolved
storybookTest(),${vitestInfo.frameworkPluginCall ? '\n' + vitestInfo.frameworkPluginCall : ''}
],
test: {
include: ['**/*.{stories}.?(m)[jt]s?(x)'],
Copy link
Contributor

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

Comment on lines +155 to +158
const builderPackageJson = await fs.readFile(
`${typeof builder === 'string' ? builder : builder.name}/package.json`,
'utf8'
);
Copy link
Contributor

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

Copy link

nx-cloud bot commented Aug 19, 2024

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

@kasperpeulen kasperpeulen marked this pull request as ready for review August 20, 2024 13:54
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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

code/addons/vitest/src/postinstall.ts Show resolved Hide resolved
storybookTest(),${vitestInfo.frameworkPluginCall ? '\n' + vitestInfo.frameworkPluginCall : ''}
],
test: {
include: ['**/*.stories.?(m)[jt]s?(x)'],
Copy link
Contributor

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

Comment on lines +203 to +206
const builderPackageJson = await fs.readFile(
`${typeof builder === 'string' ? builder : builder.name}/package.json`,
'utf8'
);
Copy link
Contributor

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

@storybook-bot
Copy link
Contributor

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

Copy link
Member

@yannbf yannbf left a 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]

code/addons/vitest/src/postinstall.ts Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/vitest/src/postinstall.ts Show resolved Hide resolved
Copy link
Member

@ghengeveld ghengeveld left a 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);
Copy link
Member

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);
Copy link
Member

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`
Copy link
Member

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);
Copy link
Member

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.

@storybook-bot
Copy link
Contributor

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

@kasperpeulen kasperpeulen merged commit ea48115 into next Aug 23, 2024
26 of 31 checks passed
@kasperpeulen kasperpeulen deleted the kasper/vitest-add branch August 23, 2024 14:15
@kasperpeulen kasperpeulen added needs qa Indicates that this needs manual QA during the upcoming minor/major release and removed needs qa Indicates that this needs manual QA during the upcoming minor/major release labels Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vitest addon: sb add postinstall hook (auto configuring the Vitest plugin)
6 participants