fix(svelte): keep PreviewRender dep-scan safe under Vite 8#34745
fix(svelte): keep PreviewRender dep-scan safe under Vite 8#34745mike-lmctl wants to merge 4 commits into
Conversation
Vite 8's Rolldown dep scanner can load Storybook's Svelte PreviewRender file through a virtual-module id. A relative sibling .svelte import from that virtual id is not resolved reliably, which prevents dependency prebundling in the Storybook Vitest addon path. Use the existing @storybook/svelte internal package export for DecoratorHandler.svelte, matching the way PreviewRender.svelte itself is loaded by callers. Add a focused renderer test so future changes do not reintroduce relative .svelte imports in PreviewRender.svelte. Tested: yarn vitest run --config code/renderers/svelte/vitest.config.ts code/renderers/svelte/src/internal-imports.test.ts
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
ChangesSvelte Internal Import Path Safety
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/renderers/svelte/src/internal-imports.test.ts (1)
25-27: ⚡ Quick winBroaden the
.svelterelative-import detection regex.Current pattern misses side-effect imports like
import './DecoratorHandler.svelte', so this test can pass with a relative.svelteimport still present.Suggested patch
- const relativeSvelteImports = [ - ...previewRender.matchAll(/from ['"](\.[^'"]+\.svelte)['"]/g), - ]; + const relativeSvelteImports = [ + ...previewRender.matchAll( + /import\s+(?:[^'"]+\s+from\s+)?['"](\.[^'"]+\.svelte)['"]/g + ), + ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/renderers/svelte/src/internal-imports.test.ts` around lines 25 - 27, The test's regex used in previewRender.matchAll (assigned to relativeSvelteImports) only detects imports that use "from '... .svelte'" and therefore misses side-effect imports like import './DecoratorHandler.svelte'; update the regex so it matches both import ... from '... .svelte' and bare side-effect imports import '... .svelte' (i.e., broaden the pattern to allow either "from '... .svelte'" or "import '... .svelte'") so relativeSvelteImports will find those additional cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/renderers/svelte/src/internal-imports.test.ts`:
- Around line 25-27: The test's regex used in previewRender.matchAll (assigned
to relativeSvelteImports) only detects imports that use "from '... .svelte'" and
therefore misses side-effect imports like import './DecoratorHandler.svelte';
update the regex so it matches both import ... from '... .svelte' and bare
side-effect imports import '... .svelte' (i.e., broaden the pattern to allow
either "from '... .svelte'" or "import '... .svelte'") so relativeSvelteImports
will find those additional cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a53e3372-d45b-4045-b509-8cc94c865a8c
📒 Files selected for processing (2)
code/renderers/svelte/src/internal-imports.test.tscode/renderers/svelte/static/PreviewRender.svelte
remove unnecessary test file
|
Thanks. Made a few modifications, will test it out. |
Closes #34304
What I did
PreviewRender.sveltenow importsDecoratorHandler.sveltethrough the existing@storybook/svelte/internal/DecoratorHandler.sveltepackage export instead of a relative sibling path.That keeps the dependency scanner out of the failing path where Vite 8/Rolldown sees
PreviewRender.sveltethrough avirtual-module:importer and then tries to resolve./DecoratorHandler.sveltefrom that virtual id.I also added a small renderer test that checks
PreviewRender.svelteuses the package export and does not add relative.svelteimports.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
I checked the focused Vite 8/Rolldown scanner path from the issue with the Windows-style virtual importer:
[UNRESOLVED_IMPORT] Could not resolve './DecoratorHandler.svelte' in virtual-module:.\node_modules\@storybook\svelte\static\PreviewRender.svelte?id=0I also ran:
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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.Summary by CodeRabbit