Maintenance: extract createPortableStoriesImpl to eliminate nextjs/nextjs-vite portable-stories duplication#34474
Conversation
…able-stories duplication
📝 WalkthroughWalkthroughA refactoring extracts Storybook portable stories composition logic into a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![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
🧹 Nitpick comments (2)
code/frameworks/nextjs/src/portable-stories-impl.ts (1)
22-23: Cross-package relative import is fragile but intentional.This relative path traverses outside the package boundary into
renderers/react. While this is documented as intentional for prebundling/ESM compatibility, it creates a coupling that could break if the directory structure changes. Consider documenting this dependency in a comment or the package README to prevent accidental breakage during future refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/frameworks/nextjs/src/portable-stories-impl.ts` around lines 22 - 23, The relative import of INTERNAL_DEFAULT_PROJECT_ANNOTATIONS from '../../../renderers/react/src/portable-stories.tsx' is intentionally fragile for prebundling/ESM reasons; update the import site (portable-stories-impl.ts) to add a clear comment documenting why this cross-package relative import is required, warn maintainers that moving or renaming the renderers/react directory will break it, and add a note in the package README (or a short TODO with a link to README) explaining the dependency and how to replace it if the repo layout changes so future refactors do not accidentally remove or relocate INTERNAL_DEFAULT_PROJECT_ANNOTATIONS.code/frameworks/nextjs-vite/src/portable-stories.ts (1)
14-18: Clean delegation to shared implementation.The import correctly uses the package specifier
@storybook/nextjs/portable-stories-impland provides the local./preview.tsxannotations, ensuring nextjs-vite-specific configuration is applied.Note: Unlike
@storybook/nextjs, this file does not re-exportINTERNAL_DEFAULT_PROJECT_ANNOTATIONS. If any consumers need access to it from@storybook/nextjs-vite, consider adding the export for API parity:-const { setProjectAnnotations: _setProjectAnnotations, composeStory: _composeStory, composeStories: _composeStories } = +const { setProjectAnnotations: _setProjectAnnotations, composeStory: _composeStory, composeStories: _composeStories, INTERNAL_DEFAULT_PROJECT_ANNOTATIONS } = createPortableStoriesImpl(nextJsAnnotations); + +export { INTERNAL_DEFAULT_PROJECT_ANNOTATIONS };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/frameworks/nextjs-vite/src/portable-stories.ts` around lines 14 - 18, The file delegates to createPortableStoriesImpl correctly but does not re-export INTERNAL_DEFAULT_PROJECT_ANNOTATIONS for API parity with `@storybook/nextjs`; update portable-stories.ts to re-export INTERNAL_DEFAULT_PROJECT_ANNOTATIONS by importing it from '@storybook/nextjs/portable-stories-impl' (or from the created impl) and adding an export alongside the existing delegated symbols (_setProjectAnnotations/_composeStory/_composeStories) so consumers of `@storybook/nextjs-vite` can access INTERNAL_DEFAULT_PROJECT_ANNOTATIONS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/frameworks/nextjs/package.json`:
- Around line 68-72: Add a build entry for the public export declared in
package.json by updating the browser build array in build-config.ts: add an
object with exportEntries containing './portable-stories-impl' and entryPoint
set to './src/portable-stories-impl.ts' so the build emits
dist/portable-stories-impl.js; locate the browser array in
code/frameworks/nextjs/build-config.ts and append the new entry object matching
the pattern used for other entries.
---
Nitpick comments:
In `@code/frameworks/nextjs-vite/src/portable-stories.ts`:
- Around line 14-18: The file delegates to createPortableStoriesImpl correctly
but does not re-export INTERNAL_DEFAULT_PROJECT_ANNOTATIONS for API parity with
`@storybook/nextjs`; update portable-stories.ts to re-export
INTERNAL_DEFAULT_PROJECT_ANNOTATIONS by importing it from
'@storybook/nextjs/portable-stories-impl' (or from the created impl) and adding
an export alongside the existing delegated symbols
(_setProjectAnnotations/_composeStory/_composeStories) so consumers of
`@storybook/nextjs-vite` can access INTERNAL_DEFAULT_PROJECT_ANNOTATIONS.
In `@code/frameworks/nextjs/src/portable-stories-impl.ts`:
- Around line 22-23: The relative import of INTERNAL_DEFAULT_PROJECT_ANNOTATIONS
from '../../../renderers/react/src/portable-stories.tsx' is intentionally
fragile for prebundling/ESM reasons; update the import site
(portable-stories-impl.ts) to add a clear comment documenting why this
cross-package relative import is required, warn maintainers that moving or
renaming the renderers/react directory will break it, and add a note in the
package README (or a short TODO with a link to README) explaining the dependency
and how to replace it if the repo layout changes so future refactors do not
accidentally remove or relocate INTERNAL_DEFAULT_PROJECT_ANNOTATIONS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f212974f-7864-45b0-88b3-3810c0fb0994
📒 Files selected for processing (5)
code/frameworks/nextjs-vite/package.jsoncode/frameworks/nextjs-vite/src/portable-stories.tscode/frameworks/nextjs/package.jsoncode/frameworks/nextjs/src/portable-stories-impl.tscode/frameworks/nextjs/src/portable-stories.ts
| "./portable-stories-impl": { | ||
| "types": "./dist/portable-stories-impl.d.ts", | ||
| "code": "./src/portable-stories-impl.ts", | ||
| "default": "./dist/portable-stories-impl.js" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that portable-stories-impl is not registered in build-config.ts
echo "=== Checking build-config.ts for portable-stories-impl entry ==="
rg -n 'portable-stories-impl' code/frameworks/nextjs/build-config.ts || echo "Entry NOT found in build-config.ts - needs to be added"Repository: storybookjs/storybook
Length of output: 184
🏁 Script executed:
# Check build-config.ts structure to understand entry points
head -100 code/frameworks/nextjs/build-config.tsRepository: storybookjs/storybook
Length of output: 2678
🏁 Script executed:
# Check if portable-stories-impl.ts file exists
ls -la code/frameworks/nextjs/src/ | grep -i portable || echo "portable-stories-impl not found in src"Repository: storybookjs/storybook
Length of output: 209
🏁 Script executed:
# Search for all references to portable-stories-impl in the codebase
rg -l 'portable-stories-impl' code/frameworks/nextjs/ 2>/dev/null || echo "No references found"Repository: storybookjs/storybook
Length of output: 147
🏁 Script executed:
# Check how portable-stories-impl is used in portable-stories.ts
cat code/frameworks/nextjs/src/portable-stories.ts | head -50Repository: storybookjs/storybook
Length of output: 1723
🏁 Script executed:
# Check the entire build-config.ts to see if there's any fallback or alternative mechanism
wc -l code/frameworks/nextjs/build-config.ts
tail -20 code/frameworks/nextjs/build-config.tsRepository: storybookjs/storybook
Length of output: 600
🏁 Script executed:
# Check if there are any imports of the public export `@storybook/nextjs/portable-stories-impl`
rg '@storybook/nextjs/portable-stories-impl' code/ --type ts --type tsx 2>/dev/null | head -20Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Also check package.json more broadly to see context around this export
sed -n '60,80p' code/frameworks/nextjs/package.jsonRepository: storybookjs/storybook
Length of output: 825
Missing build entry point will cause runtime errors when importing the public export.
The ./portable-stories-impl export is declared in package.json (pointing to ./dist/portable-stories-impl.js) but is not registered in code/frameworks/nextjs/build-config.ts. The corresponding dist file will not be generated during the build process, causing MODULE_NOT_FOUND errors at runtime when external consumers import from @storybook/nextjs/portable-stories-impl.
Add the entry point to code/frameworks/nextjs/build-config.ts:
Suggested addition to the browser array:
{
exportEntries: ['./portable-stories-impl'],
entryPoint: './src/portable-stories-impl.ts',
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/frameworks/nextjs/package.json` around lines 68 - 72, Add a build entry
for the public export declared in package.json by updating the browser build
array in build-config.ts: add an object with exportEntries containing
'./portable-stories-impl' and entryPoint set to './src/portable-stories-impl.ts'
so the build emits dist/portable-stories-impl.js; locate the browser array in
code/frameworks/nextjs/build-config.ts and append the new entry object matching
the pattern used for other entries.
|
Closing due to #34453 (comment) |
2 participants
Closes #34470
What I did
portable-stories.tswas duplicated almost entirely between@storybook/nextjsand@storybook/nextjs-vite. The two files were 143 lines long and differed only in 3 JSDoc comment lines referencing the package name in usage examples.Created
createPortableStoriesImpl(nextJsAnnotations)factory incode/frameworks/nextjs/src/portable-stories-impl.ts. Both frameworks now call this factory with their own./preview.tsxannotations and re-wrap each function with their package-specific JSDoc.Changes:
code/frameworks/nextjs/src/portable-stories-impl.ts— shared implementation factorycode/frameworks/nextjs/src/portable-stories.ts— thin wrapper with@storybook/nextjsJSDoccode/frameworks/nextjs-vite/src/portable-stories.ts— thin wrapper with@storybook/nextjs-viteJSDoc./portable-stories-implexport entry to@storybook/nextjs@storybook/nextjs: workspace:*to@storybook/nextjs-vitedependenciesRuntime behavior is unchanged — each package still uses its own
preview.tsxto buildINTERNAL_DEFAULT_PROJECT_ANNOTATIONS, and all public APIs have identical type signatures.Checklist for Contributors
Testing
Manual testing
This is a pure refactor — no behavior changes. Tested by reviewing that the public API types and exports remain identical.
Summary by CodeRabbit