-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
CSF: Support .Component property in extended stories for React #32756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…sts for extended stories
📝 WalkthroughWalkthroughThis PR extends CSF stories with an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/csf/csf-factories.ts (1)
256-260: Consider moving renderer-specific logic to the renderer layer.This code adds React-specific Component assignment logic to the core CSF layer, creating coupling between the core and a specific renderer. While the approach is pragmatic and consistent with the existing pattern in
code/renderers/react/src/preview.tsx(line 47), it violates separation of concerns.Additionally, there's a TODO comment in
preview.tsx(line 44) questioning whether this Component construct is needed:"Are we sure we want this? the Component construct was for compatibility with raw portable stories. We don't actually use this in vitest."Consider:
- Having the React renderer override the
extendmethod to handle Component assignment- Implementing a hook/callback system where renderers can customize extend behavior
- Keeping core CSF logic renderer-agnostic
That said, if this coupling is intentional for pragmatic reasons (e.g., avoiding duplication across multiple renderers), the current implementation is correct and the conditional check prevents errors for non-React renderers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/core/src/csf/csf-factories.ts(2 hunks)code/renderers/react/src/__test__/portable-stories-factory.test.tsx(1 hunks)code/renderers/react/src/csf-factories.test.tsx(1 hunks)code/renderers/react/src/preview.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsxcode/renderers/react/src/__test__/portable-stories-factory.test.tsxcode/core/src/csf/csf-factories.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsxcode/renderers/react/src/__test__/portable-stories-factory.test.tsxcode/core/src/csf/csf-factories.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsxcode/renderers/react/src/__test__/portable-stories-factory.test.tsxcode/core/src/csf/csf-factories.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsxcode/renderers/react/src/__test__/portable-stories-factory.test.tsxcode/core/src/csf/csf-factories.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/renderers/react/src/csf-factories.test.tsxcode/renderers/react/src/__test__/portable-stories-factory.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/renderers/react/src/csf-factories.test.tsxcode/renderers/react/src/__test__/portable-stories-factory.test.tsx
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/renderers/react/src/csf-factories.test.tsxcode/renderers/react/src/__test__/portable-stories-factory.test.tsx
🧬 Code graph analysis (1)
code/renderers/react/src/__test__/portable-stories-factory.test.tsx (1)
code/renderers/svelte/src/__test__/composeStories/Button.stories.ts (1)
CSF3Primary(95-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/renderers/react/src/__test__/portable-stories-factory.test.tsx (1)
105-112: LGTM! Test verifies extend functionality with merged args.The test correctly verifies that:
CSF3Primary.extend()creates a new story with merged args- The extended story's
Componentproperty is defined and renders correctly- The updated
childrenarg is reflected in the rendered outputcode/renderers/react/src/preview.tsx (1)
120-122: LGTM! Type signature correctly extends Story.extend for React.The method signature properly:
- Constrains the input to
StoryAnnotations<T, T['args']>- Returns a narrowed
ReactStory<T, TNewInput>type- Maintains type safety for the extend functionality
code/renderers/react/src/csf-factories.test.tsx (1)
44-49: LGTM! Runtime checks verify Component property on extended stories.The tests properly validate:
MyStory.Componentis defined after story creationMyStory.extend()returns a new story with merged argsExtendedMyStory.Componentis also definedThis provides good runtime coverage of the new extend API.
|
View your CI Pipeline Execution ↗ for commit bd00e44
☁️ Nx Cloud last updated this comment at |
|
@storybookjs/core could I get a canary of this? I believe it's necessary for me to migrate to CSF factories. Edit: never mind, I can use the private |
|
Closing this as not implemented, we can revisit this later |
|
Yann, what is the reason not to implement this? I'm a bit nervous now, because I migrated my codebase using the undocumented private method expecting this PR would merge and I could change to .Component. |
Hey there! This PR was about an experimental feature we thought of providing. There's a lot to unpack here:
In the meantime, we have decided to park this. You can still use portable stories though, here are examples you can follow: import { composeStory } from '@storybook/react-vite'
const meta = preview.meta({})
const Primary = meta.story({})
const Secondary = meta.story({})
const composed = [Primary, Secondary].map((story) => {
return composeStory(story.input, meta.input, meta.preview.composed)
});
// or using this internal API (for now)
const composed = [Primary, Secondary].map((story) => {
return story.__compose()
}); |
Closes #
What I did
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 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
extend()method to create variants with custom merged argumentsComponentproperty on stories for direct component access