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: Fix add command #28975

Merged
merged 6 commits into from
Aug 27, 2024
Merged

Vitest: Fix add command #28975

merged 6 commits into from
Aug 27, 2024

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Aug 26, 2024

What I did

Fixes the add command for addon-vitest to write a workspace config file if we find a vite.config.ts/js file, and extend our test config from that file. Removed the isolate: false option because it causes errors regarding empty story files (no tests).

Also cleans up a few smaller issues.

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-28975-sha-d3d6fd2e. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information
Published version 0.0.0-pr-28975-sha-d3d6fd2e
Triggered by @kasperpeulen
Repository storybookjs/storybook
Branch vitest-addon-fixes
Commit d3d6fd2e
Datetime Tue Aug 27 11:15:46 UTC 2024 (1724757346)
Workflow run 10577025074

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=28975

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 76.4 MB 76.4 MB 0 B 0.73 0%
initSize 150 MB 169 MB 18.9 MB 0.24 11.2%
diffSize 73.9 MB 92.8 MB 18.9 MB 0.23 20.4%
buildSize 7.46 MB 7.46 MB -267 B 0.98 0%
buildSbAddonsSize 1.62 MB 1.62 MB 0 B 0.86 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.3 MB 2.3 MB 0 B - 0%
buildSbPreviewSize 352 kB 352 kB 0 B 1 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.46 MB 4.46 MB 0 B 0.93 0%
buildPreviewSize 3 MB 3 MB -267 B 0.96 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 7.2s 7.4s 160ms -1.25 2.2%
generateTime 22.7s 20.2s -2s -456ms -0.38 -12.1%
initTime 17.2s 16.1s -1s -46ms -1.17 -6.5%
buildTime 11.3s 12.2s 936ms -0.25 7.6%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 7.5s 6.6s -867ms -1.02 -13.1%
devManagerResponsive 4.7s 4.3s -368ms -1.17 -8.5%
devManagerHeaderVisible 832ms 779ms -53ms -0.31 -6.8%
devManagerIndexVisible 861ms 813ms -48ms -0.34 -5.9%
devStoryVisibleUncached 888ms 911ms 23ms -1.92 2.5%
devStoryVisible 872ms 814ms -58ms -0.35 -7.1%
devAutodocsVisible 743ms 648ms -95ms -0.93 -14.7%
devMDXVisible 864ms 753ms -111ms 0.41 -14.7%
buildManagerHeaderVisible 891ms 696ms -195ms -0.7 -28%
buildManagerIndexVisible 893ms 703ms -190ms -0.7 -27%
buildStoryVisible 940ms 737ms -203ms -0.84 -27.5%
buildAutodocsVisible 832ms 607ms -225ms -1.58 🔰-37.1%
buildMDXVisible 683ms 601ms -82ms -1.07 -13.6%

Greptile Summary

This PR enhances the Vitest addon's postinstall script for Storybook, improving compatibility and configuration handling.

  • Improved detection of existing Vite and Vitest config files
  • Added support for creating workspace configurations when existing configs are found
  • Enhanced handling for Next.js projects, including automatic plugin configuration
  • Streamlined creation of new Vitest config file when no existing configs are present
  • Improved user guidance and logging throughout the installation process

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.

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

nx-cloud bot commented Aug 26, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 534fb2b. 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 changed the title Vitest addon fixes Vitest: Fix add command Aug 27, 2024
export default defineWorkspace([
'${relative(dirname(browserWorkspaceFile), rootConfig)}',
{
extends: '${viteConfig || ''}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no viteConfig it now extends an empty string.
But in that case, I think we should just not extend anything

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we want to make this a relative path, similar as the one above:
'${relative(dirname(browserWorkspaceFile), rootConfig)}',

]);
`
]);
`.replace(/\s+extends: '',/, '')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see

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.

2 participants