CSF-Factories: Fix codemod for preview files without exports#33673
Conversation
…ly preview files Reproduces issue #33640: When a preview file only has side-effect imports (like `import './preview.scss'`) or is empty, the codemod doesn't add a default export, which breaks story files that import from preview.
When a preview file only has side-effect imports (like `import './preview.scss'`)
or is empty, the codemod now correctly adds `export default definePreview({})`.
This is needed because story files using CSF factories import from preview,
so the preview file must have a default export.
Fixes #33640
kasperpeulen
changed the title
|
View your CI Pipeline Execution ↗ for commit b9b7036
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds handling for preview config files with no exports (empty or only side-effect imports) to the CSF factory codemod by injecting a Changes
Sequence Diagram(s)(Skipped — change is a codemod/local transformation, not a multi-component runtime flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing touches
Comment |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 776 KB | 776 KB | 0 B |
| Dependency size | 67.57 MB | 67.56 MB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts`:
- Around line 168-182: The preview-case branch currently always appends an
export default defineConfig call when configType === 'preview', which can create
duplicate exports for files that actually contain exports (e.g., re-exports);
update the guard to only run this branch when there are truly no export
declarations by checking exportDecls.length === 0 (or equivalent) before
creating defineConfigCall and pushing exportDefaultDeclaration onto
programNode.body (referencing configType, exportDecls, methodName,
defineConfigCall, and programNode).
shilman
left a comment
shilman
left a comment
There was a problem hiding this comment.
A few minor comments but otherwise LGTM
…s with re-exports
This addresses the CodeRabbit review comment. The original code would add
`export default definePreview({})` for any preview file that didn't have
a default export or recognized named exports, which could cause duplicate
export errors for files with re-exports like `export { x } from './module'`.
The fix checks if the AST has any exports at all (including re-exports
and `export *` declarations) before adding the default export.
…iew files with re-exports" This reverts commit 1aec190.
|
Addressed the review comments: @coderabbitai's suggestion: After further analysis, the original code is correct. The CodeRabbit suggestion was based on a misunderstanding:
@shilman's questions:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.ts (1)
196-227: Minor: Import is added unconditionally, even when unused.When a preview file contains only re-exports (handled by the
hasAnyExportscheck preventing Scenario 4), thedefinePreviewimport is still added but never used. This would trigger linting warnings.Consider tracking whether a
defineConfigCallwas created and conditionally adding the import.🔧 Suggested approach
+ let needsDefineImport = false; + if (config._exportsObject && hasNamedExports) { + needsDefineImport = true; // ... existing code } else if (config._exportsObject) { + needsDefineImport = true; // ... existing code } else if (hasNamedExports) { + needsDefineImport = true; // ... existing code } else if (configType === 'preview') { // ... if (!hasAnyExports) { + needsDefineImport = true; // ... existing code } } - const configImport = t.importDeclaration( + if (!needsDefineImport) { + // Skip import if defineMain/definePreview wasn't used + const output = printConfig(config).code; + // ... rest of output handling + } + + const configImport = t.importDeclaration(
|
Tip For best results, initiate chat on the files or code changes.
I now understand that:
Your explanation about why this handling is only needed for preview files (not main files) also makes sense. Thank you for taking the time to clarify this behavior! ✏️ Learnings added
🧠 Learnings used |
kasperpeulen
added
the
patch:yes
…-exports CSF-Factories: Fix codemod for preview files without exports (cherry picked from commit 0d867e4)
github-actions
Bot
added
the
patch:done
2 participants
Closes #33640
What I did
Fixed the CSF factories codemod to handle preview files that have no exports (only side-effect imports or empty files).
When a preview file only has
import './preview.scss'or is empty, the codemod now correctly addsexport default definePreview({}).Problem: Story files using CSF factories import from preview:
But if the preview file only has side-effect imports or is empty, the codemod wouldn't add a default export, causing broken imports.
Checklist 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!
No manual testing necessary - the fix is fully covered by the 3 new unit tests:
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.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.