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

Addon Test: Merge viteFinal config into vitest config #29806

Merged
merged 9 commits into from
Dec 10, 2024

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Dec 4, 2024

Closes #29778

What I did

I use the preset apply mechanism to load the viteFinal caonfig value, then merge it into the vitest config

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

I manually verified this by modifying a story with an import that did not exist.
Then adding an alias for this non-existing import in the main.ts viteFinal config.

That worked.

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-storybook/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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 77.7 MB 77.7 MB 0 B -0.15 0%
initSize 133 MB 133 MB 0 B 1.99 0%
diffSize 55.1 MB 55.1 MB 0 B 1.99 0%
buildSize 6.87 MB 6.87 MB 0 B 1 0%
buildSbAddonsSize 1.51 MB 1.51 MB 0 B 2.11 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.86 MB 1.86 MB 0 B 2.96 0%
buildSbPreviewSize 0 B 0 B 0 B - -
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.57 MB 3.57 MB 0 B 2.64 0%
buildPreviewSize 3.3 MB 3.3 MB 0 B 0.94 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.1s 8.7s 1.6s -0.21 18.8%
generateTime 19.9s 24.5s 4.6s 1.88 🔺19%
initTime 13.1s 14.4s 1.2s 0.34 8.7%
buildTime 9.1s 8.8s -270ms 0.04 -3.1%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 5.6s 4.5s -1s -133ms -0.99 -24.9%
devManagerResponsive 4.2s 3.5s -690ms -0.59 -19.3%
devManagerHeaderVisible 537ms 467ms -70ms -1.67 🔰-15%
devManagerIndexVisible 566ms 494ms -72ms -1.59 🔰-14.6%
devStoryVisibleUncached 1.3s 970ms -347ms -2.24 🔰-35.8%
devStoryVisible 572ms 537ms -35ms -1.12 -6.5%
devAutodocsVisible 665ms 430ms -235ms -1.26 🔰-54.7%
devMDXVisible 514ms 435ms -79ms -1.15 -18.2%
buildManagerHeaderVisible 669ms 513ms -156ms -0.85 -30.4%
buildManagerIndexVisible 821ms 588ms -233ms -0.9 -39.6%
buildStoryVisible 610ms 464ms -146ms -0.93 -31.5%
buildAutodocsVisible 526ms 406ms -120ms -0.86 -29.6%
buildMDXVisible 416ms 375ms -41ms -1.31 🔰-10.9%

Greptile Summary

This PR adds functionality to merge Storybook's viteFinal configuration into the Vitest config, ensuring custom Vite configurations are properly applied during test runs.

  • Added mergeConfig import from vitest/config in /code/addons/test/src/vitest-plugin/index.ts
  • Implemented presets.apply('viteFinal') to load custom Vite configuration from Storybook's main config
  • Merged Storybook's Vite config with Vitest config using mergeConfig(viteConfigFromStorybook, config)
  • Added framework-specific optimizations for React/Next.js and Vue3 configurations
  • Preserved existing test configuration properties while merging new ones from Storybook

Copy link

nx-cloud bot commented Dec 4, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 46743b4. 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


🟥 Failed Commands
nx run-many -t build --parallel=3

Sent with 💌 from NxCloud.

@ndelangen ndelangen marked this pull request as ready for review December 4, 2024 11:55
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.

1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
Base automatically changed from yann/automatic-stories-glob to next December 4, 2024 13:02
@yannbf
Copy link
Member

yannbf commented Dec 4, 2024

Edit: I think the plugin bit won't work. Still I feel like we need to add a migration note for users who already added custom stuff themselves

@ndelangen I'm not sure if the plugins from frameworks will be included (like a Vite plugin probably can't add other plugins), but if they are, we need to test if this works for svelte, vue3, react-native-web-vite and nextjs without the need of setting the vite plugin like it is described currently in the docs:
https://storybook.js.org/docs/writing-tests/test-addon#framework-plugins

So if it works then:
- the docs need to be reworked
- the postinstall script needs to not do the logic to inject those framework plugins as it does not
- we need to make sure that we notify users that their configs need to change (the framework plugins would not be needed)

@JReinhold
Copy link
Contributor

This limitation is going to hurt us:

image

from https://vite.dev/guide/api-plugin.html#config

We can modify the config as much as we want, but any modifications to the plugins array won't have an affect. Internally we make heavy use of adding Vite-specifics via plugins, and it's also the recommended pattern in userland, so this is not an edge case. It means that even with this work here, we're still not getting the actual Storybook Vite configuration into the Vitest runtime. And so I wonder, if it's best to try or not?

  1. Either we keep this: https://storybook.js.org/docs/writing-tests/test-addon#how-do-i-apply-custom-vite-configuration
  2. ... or we say, "Most of your viteFinal is respected, except for plugins, you still have to copy them over to your Vitest config"

@kylegach
Copy link
Contributor

kylegach commented Dec 4, 2024

From what I've been able to observe, most of the viteFinal that is needed is aliases and resolvers. So I think we should keep those docs, but revise them to make it clear that it's only necessary for plugins.

@JReinhold
Copy link
Contributor

There might actually be a way around the plugins limitations I explained above.

We can't add plugins in the config hook, but we can make the overall storybookTest function return multiple plugins. So we could resolve the viteFinal preset upfront, find all plugins returned, and return them from the function, next to our plugin - and then the rest of the config would be in added in the config hook.

We don't need to do this now, let's ship what we have, but this could be a good solution when working on storybookjs/addon-svelte-csf#213

@ndelangen ndelangen requested a review from ghengeveld December 9, 2024 11:43
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

previously we did some special handling for some of the properties, which we still need to do.

code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
code/addons/test/src/vitest-plugin/index.ts Outdated Show resolved Hide resolved
@storybook-pr-benchmarking
Copy link

storybook-pr-benchmarking bot commented Dec 10, 2024

Package Benchmarks

Commit: 46743b4, ran on 10 December 2024 at 12:59:40 UTC

No significant changes detected, all good. 👏

@JReinhold JReinhold merged commit 8872560 into next Dec 10, 2024
9 of 23 checks passed
@JReinhold JReinhold deleted the norbert/custom-vitest-config-support branch December 10, 2024 12:54
@github-actions github-actions bot mentioned this pull request Dec 10, 2024
11 tasks
@shilman shilman changed the title TestAddon: Merge viteFinal config into vitest config Addon Test: Merge viteFinal config into vitest config Dec 10, 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.

[Bug] custom configuration ignored by Vitest plugin
5 participants