Addon-Vitest: Streamline vite(st) config detection across init and postinstall#34193
Conversation
|
View your CI Pipeline Execution ↗ for commit 4ef9a1e
☁️ Nx Cloud last updated this comment at |
…ion-across-init-and-postinstall
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughExtracts Vitest/Vite AST helpers into a new shared module, re-exports them from core babel, refactors addon-vitest and AddonVitestService to consume those helpers, adds tests for the helpers, and removes an unused import. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
📝 Coding Plan
Comment |
yannbf
left a comment
There was a problem hiding this comment.
Code looks and works well, so LGTM but I feel like it's weird to have vitest helpers exposed as APIs from storybook/internal/babel. Some of these utilities are generic enough and could be nice to have, but vitest specific things do feel a little weird to me. Last time we had vitest specific code (transformer) we moved it to csf-tools.
I kind of share the same feeling (but not too strongly). I am okay to move it into whatever core package you prefer. Since it is heavily based on Babel, I assumed |
befc63a to
4ef9a1e
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.45 MB | 20.46 MB | 🚨 +13 KB 🚨 |
| Dependency size | 16.54 MB | 16.54 MB | 🎉 -2 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 121 | 124 | 🚨 +3 🚨 |
| Self size | 30 KB | 30 KB | 🎉 -5 B 🎉 |
| Dependency size | 23.54 MB | 23.78 MB | 🚨 +240 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 779 KB | 779 KB | 🎉 -71 B 🎉 |
| Dependency size | 67.65 MB | 67.66 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -2 B 🎉 |
| Dependency size | 66.17 MB | 66.19 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1.05 MB | 1.04 MB | 🎉 -2 KB 🎉 |
| Dependency size | 36.99 MB | 37.01 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | node | node |
My perspective is that internal/babel should hold core utilities that are low-level only (which is how it stands now). One alternative would be to make the utilities more low-level/generic and leave them in internal/babel, but we can just move to csf-tools and call it a day, then the PR is good to merge 👍 |
|
csf-tools, on the other hand, are about "CSF tools". The helpers we talk about aren't about story configs or main/preview configs at all. Don't you think that by taking the domain into consideration, |
…-validation-detection-across-init-and-postinstall Addon-Vitest: Streamline vite(st) config detection across init and postinstall (cherry picked from commit b2fb7d8)
Closes #
What I did
Extracted the shared AST helper functions for analyzing Vitest/Vite config files into a new
vitest-config-helpers.tsmodule insidestorybook/internal/babel. Previously, bothAddonVitestService(used duringstorybook init) andupdateVitestFile(used during the postinstall script) each had their own copy of helpers likeisDefineConfigLike,getConfigObjectFromMergeArg,getEffectiveMergeConfigCall, andgetTargetConfigObject.This PR:
code/core/src/babel/vitest-config-helpers.tswith the shared helpers, plus two new top-level functionscanUpdateVitestConfigFileandcanUpdateVitestWorkspaceFilestorybook/internal/babelupdateVitestFile.tsandAddonVitestService.ts, replacing them with importsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
This is a pure refactor that moves code without changing behavior. The shared helpers are tested via unit tests in
vitest-config-helpers.test.ts. The existing integration paths (storybook initand the addon postinstall) continue to use the same logic, just from a shared location.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-34193-sha-7695cbc2. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34193-sha-7695cbc2 sandboxor in an existing project withnpx storybook@0.0.0-pr-34193-sha-7695cbc2 upgrade.More information
0.0.0-pr-34193-sha-7695cbc2valentin/streamline-config-validation-detection-across-init-and-postinstall7695cbc21773829183)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=34193Summary by CodeRabbit
Tests
Refactor