Conversation
…was trying to optimize the vitest plugin itself)
JReinhold
changed the title
viteFinal
|
View your CI Pipeline Execution ↗ for commit f7285f0.
☁️ Nx Cloud last updated this comment at |
JReinhold
added
feature request
svelte
addon: vitest
ci:daily
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
| } | ||
|
|
||
| return config; | ||
| return { ...config, plugins }; |
There was a problem hiding this comment.
before, this preset would discard any config set by previous viteFinal presets. We never noticed it because this would always run first in Storybook. But in the Vitest plugin it appears that main.ts runs first, so we need to handle it properly here.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
…pport-svelte-csf-in-vitest
…pport-svelte-csf-in-vitest
| import type { PluginOption } from 'vite'; | ||
|
|
||
| // ! Relative import to prebundle it without needing to depend on the Vite builder | ||
| import { withoutVitePlugins } from '../../../../builders/builder-vite/src/utils/without-vite-plugins'; |
There was a problem hiding this comment.
without-vite-plugins uses a type import from vite. That means that the experimental-addon-test also depends on Vite; otherwise, the PluginOption cannot be resolved in package manager environments, which are strict about peer dependencies.
Generally, I like to avoid relative imports like this because of the likelihood is high that the package that imports from another package utils or functions doesn't get the dependencies right. It can easily be overseen.
If I were you, I would copy-paste the helper function and remove the PluginOptions reference. Otherwise, experimental-addon-test needs to mention vite as a peer dependency.
There was a problem hiding this comment.
But type imports are stripped from the dist, so it wouldn't show up in the package, so it's not necessary. Unless the imported type made its way into a generated .d.ts file, but it doesn't because it's not part of the public API.
There was a problem hiding this comment.
Right, I am talking about the .d.ts file. Maybe you can check, whether a vite import is part of it.
There was a problem hiding this comment.
But it's fair to be defensive and say maybe in the future that module would import from dependencies that were in the original package but not the sibling package, without considering the sibling package.
3 participants
Closes storybookjs/addon-svelte-csf#213, follow-up to #29806 (comment)
What I did
To support Svelte CSF stories, we're now also including Vite plugins collected from the
viteFinalpreset (and thus addons) in our Vitest plugin. So now, instead of just returning a plugin, we're returning a list of plugins (which is totally valid). With the other changes to@storybook/addon-svelte-csflanding (storybookjs/addon-svelte-csf#244, storybookjs/addon-svelte-csf#247, storybookjs/addon-svelte-csf#248) this is all that's necessary to add automatic support forstories.sveltefiles.This PR also cleans up our
viteFinals frameworks, because some of them were mutating configs and plugins arrays which could cause issues if the presets were loaded in another order than expected.Things to consider:
This also adds all other plugins, like Vite docgen plugins (not just for Svelte, but also React, etc.) which could incur an unnecessary perf penalty. Do we want to handle that as part of this, or later, and how? (make it work, then make it fast?)EDIT: fixed, explicitly removing a hard coded list of known unnecessary plugins.enforce: 'pre'from the Vitest plugin, meaning that it would be more affected by changes to the plugin order. We need to QA this, but currently I can't see how this would cause issues.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: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-30105-sha-a2609294. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-30105-sha-a2609294 sandboxor in an existing project withnpx storybook@0.0.0-pr-30105-sha-a2609294 upgrade.More information
0.0.0-pr-30105-sha-a2609294jeppe/support-svelte-csf-in-vitesta26092941736331560)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 canary-release-pr.yml --field pr=30105Greptile Summary
Based on the provided information, I'll create a concise summary of the key changes in this pull request:
Adds support for Svelte CSF stories in Vitest by modifying the Storybook test plugin architecture.
storybookTestin/code/addons/test/src/vitest-plugin/index.tsto return multiple plugins and include Vite plugins fromviteFinalenforce: 'pre'setting from Vitest plugin to improve plugin compatibilityThe changes enable automatic support for
.stories.sveltefiles while maintaining a cleaner plugin architecture, though there are potential performance implications from including additional plugins that need to be monitored.