-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
CSF: Add support for CSF factories #30197
Conversation
…ries' into kasper/csf-factories
…ries' into kasper/csf-factories
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.
20 file(s) reviewed, 19 comment(s)
Edit PR Review Bot Settings | Greptile
const [previewFileUrl, ...previewAnnotationURLs] = [...previewAnnotations, previewOrConfigFile] | ||
.filter(Boolean) | ||
.map((path) => processPreviewAnnotation(path, projectRoot)); |
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.
logic: Destructuring could fail if previewAnnotations and previewOrConfigFile are both empty arrays after filtering. Add a check to handle this case.
const preview = await import('${previewFileUrl}'); | ||
const csfFactoryPreview = Object.values(preview).find(module => { | ||
return 'isCSFFactoryPreview' in module | ||
}); |
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.
logic: Object.values() and find() could return undefined. Need type checking and error handling for when no CSF factory preview is found.
const configs = await Promise.all([${previewAnnotationURLs | ||
.map( | ||
(previewAnnotation, index) => | ||
// Prefer the updated module from an HMR update, otherwise import the original module | ||
`hmrPreviewAnnotationModules[${index}] ?? import('${previewAnnotation}')` | ||
) | ||
.join(',\n')}]) | ||
return composeConfigs(configs); | ||
return composeConfigs([...configs, preview]); |
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.
logic: preview is used in composeConfigs even when csfFactoryPreview is found, which could cause conflicts. Should only include preview in non-factory case.
@@ -27,11 +27,12 @@ export const testStory = ( | |||
return async (context: TestContext & TaskContext & { story: ComposedStoryFn }) => { | |||
const composedStory = composeStory( | |||
story, | |||
meta, | |||
'isCSFFactory' in story ? (meta as any).annotations : meta, |
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.
style: Type casting to 'any' should be avoided. Consider creating proper type definitions for CSF factories to maintain type safety.
{ initialGlobals: (await getInitialGlobals?.()) ?? {} }, | ||
undefined, | ||
exportName | ||
); | ||
|
||
if (composedStory === undefined || skipTags?.some((tag) => composedStory.tags.includes(tag))) { |
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.
logic: Optional chaining on skipTags is unnecessary since it's a required parameter
const div = document.createElement('div'); | ||
document.body.appendChild(div); | ||
|
||
await CSF3InputFieldFilled.run({ canvasElement: div }); | ||
|
||
const input = screen.getByTestId('input') as HTMLInputElement; | ||
expect(input.value).toEqual('Hello world!'); | ||
|
||
document.body.removeChild(div); |
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.
style: Test cleanup could fail if test errors before removeChild. Consider using try/finally
import { Button } from './Button'; | ||
|
||
// eslint-disable-next-line storybook/default-exports | ||
const config = defineConfig({ args: { children: 'TODO: THIS IS NOT WORKING YET' } }); |
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.
logic: TODO comment indicates unfinished/broken functionality. This should be resolved before merging.
}); | ||
|
||
export const CSF2StoryWithParamsAndDecorator = meta.story({ | ||
render: (args: any) => { |
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.
style: Using any
type here loses type safety. Consider using proper Button component prop types.
code/renderers/react/src/preview.tsx
Outdated
readonly meta = <TComponent extends ComponentType<any>, TMetaArgs extends Args>( | ||
meta: ComponentAnnotations<TRenderer, any> & { component: TComponent; args: TMetaArgs } | ||
) => { |
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.
logic: The meta method accepts any for TRenderer annotations but then casts to TMetaArgs. This could lead to type safety issues. Consider constraining the any type.
code/renderers/react/src/preview.tsx
Outdated
public meta: Meta<TRenderer, TArgs>, | ||
public config: PreviewConfig<TRenderer> | ||
) { | ||
Object.assign(this, annotations); |
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.
style: Using Object.assign to copy all properties from annotations could lead to property collisions with the class's own methods/properties. Consider explicit property copying instead.
View your CI Pipeline Execution ↗ for commit a73f656.
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
Before | After | Difference | |
---|---|---|---|
Dependency count | 54 | 54 | 0 |
Self size | 332 KB | 444 KB | 🚨 +112 KB 🚨 |
Dependency size | 12.46 MB | 12.46 MB | 🚨 +3 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/addon-actions
Before | After | Difference | |
---|---|---|---|
Dependency count | 7 | 7 | 0 |
Self size | 59 KB | 65 KB | 🚨 +6 KB 🚨 |
Dependency size | 3.22 MB | 3.22 MB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/addon-backgrounds
Before | After | Difference | |
---|---|---|---|
Dependency count | 4 | 4 | 0 |
Self size | 27 KB | 45 KB | 🚨 +18 KB 🚨 |
Dependency size | 99 KB | 99 KB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/addon-essentials
Before | After | Difference | |
---|---|---|---|
Dependency count | 32 | 32 | 0 |
Self size | 12 KB | 18 KB | 🚨 +6 KB 🚨 |
Dependency size | 15.44 MB | 15.53 MB | 🚨 +88 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/addon-highlight
Before | After | Difference | |
---|---|---|---|
Dependency count | 1 | 1 | 0 |
Self size | 9 KB | 12 KB | 🚨 +3 KB 🚨 |
Dependency size | 5 KB | 5 KB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/addon-measure
Before | After | Difference | |
---|---|---|---|
Dependency count | 2 | 2 | 0 |
Self size | 32 KB | 60 KB | 🚨 +28 KB 🚨 |
Dependency size | 20 KB | 20 KB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/addon-outline
Before | After | Difference | |
---|---|---|---|
Dependency count | 2 | 2 | 0 |
Self size | 23 KB | 42 KB | 🚨 +19 KB 🚨 |
Dependency size | 32 KB | 32 KB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/addon-viewport
Before | After | Difference | |
---|---|---|---|
Dependency count | 2 | 2 | 0 |
Self size | 27 KB | 30 KB | 🚨 +4 KB 🚨 |
Dependency size | 67 KB | 67 KB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/core
Before | After | Difference | |
---|---|---|---|
Dependency count | 52 | 52 | 0 |
Self size | 19.20 MB | 19.24 MB | 🚨 +39 KB 🚨 |
Dependency size | 14.19 MB | 14.19 MB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/experimental-nextjs-vite
Before | After | Difference | |
---|---|---|---|
Dependency count | 187 | 187 | 0 |
Self size | 232 KB | 235 KB | 🚨 +3 KB 🚨 |
Dependency size | 48.02 MB | 48.83 MB | 🚨 +805 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/html-vite
Before | After | Difference | |
---|---|---|---|
Dependency count | 15 | 15 | 0 |
Self size | 5 KB | 6 KB | 🚨 +1 KB 🚨 |
Dependency size | 1.91 MB | 1.91 MB | 🚨 +604 B 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/html-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 247 | 247 | 0 |
Self size | 6 KB | 8 KB | 🚨 +1 KB 🚨 |
Dependency size | 31.72 MB | 31.72 MB | 🚨 +320 B 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/nextjs
Before | After | Difference | |
---|---|---|---|
Dependency count | 593 | 593 | 0 |
Self size | 472 KB | 475 KB | 🚨 +3 KB 🚨 |
Dependency size | 83.66 MB | 84.47 MB | 🚨 +803 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/preact-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 245 | 245 | 0 |
Self size | 6 KB | 8 KB | 🚨 +1 KB 🚨 |
Dependency size | 31.30 MB | 31.30 MB | 🚨 +320 B 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
Before | After | Difference | |
---|---|---|---|
Dependency count | 177 | 177 | 0 |
Self size | 43 KB | 44 KB | 🚨 +1 KB 🚨 |
Dependency size | 23.49 MB | 24.29 MB | 🚨 +805 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/react-vite
Before | After | Difference | |
---|---|---|---|
Dependency count | 129 | 129 | 0 |
Self size | 13 KB | 15 KB | 🚨 +2 KB 🚨 |
Dependency size | 19.67 MB | 20.47 MB | 🚨 +803 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 324 | 324 | 0 |
Self size | 6 KB | 8 KB | 🚨 +2 KB 🚨 |
Dependency size | 42.54 MB | 43.34 MB | 🚨 +803 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 253 | 253 | 0 |
Self size | 14 KB | 15 KB | 🚨 +1 KB 🚨 |
Dependency size | 32.45 MB | 32.45 MB | 🚨 +320 B 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/svelte-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 311 | 311 | 0 |
Self size | 6 KB | 7 KB | 🚨 +1 KB 🚨 |
Dependency size | 39.34 MB | 39.34 MB | 🚨 +320 B 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/vue3-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 496 | 496 | 0 |
Self size | 6 KB | 7 KB | 🚨 +1 KB 🚨 |
Dependency size | 55.73 MB | 55.73 MB | 🚨 +320 B 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/web-components-vite
Before | After | Difference | |
---|---|---|---|
Dependency count | 16 | 16 | 0 |
Self size | 5 KB | 6 KB | 🚨 +1 KB 🚨 |
Dependency size | 1.94 MB | 1.94 MB | 🚨 +604 B 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/web-components-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 245 | 245 | 0 |
Self size | 5 KB | 7 KB | 🚨 +1 KB 🚨 |
Dependency size | 31.35 MB | 31.35 MB | 🚨 +320 B 🚨 |
Bundle Size Analyzer | Link | Link |
storybook
Before | After | Difference | |
---|---|---|---|
Dependency count | 53 | 53 | 0 |
Self size | 23 KB | 24 KB | 🚨 +710 B 🚨 |
Dependency size | 33.39 MB | 33.43 MB | 🚨 +39 KB 🚨 |
Bundle Size Analyzer | Link | Link |
sb
Before | After | Difference | |
---|---|---|---|
Dependency count | 54 | 54 | 0 |
Self size | 1 KB | 1 KB | 0 B |
Dependency size | 33.41 MB | 33.45 MB | 🚨 +40 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/cli
Before | After | Difference | |
---|---|---|---|
Dependency count | 358 | 360 | 🚨 +2 🚨 |
Self size | 261 KB | 278 KB | 🚨 +17 KB 🚨 |
Dependency size | 83.85 MB | 83.91 MB | 🚨 +63 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/codemod
Before | After | Difference | |
---|---|---|---|
Dependency count | 276 | 276 | 0 |
Self size | 612 KB | 612 KB | 0 B |
Dependency size | 65.45 MB | 65.49 MB | 🚨 +39 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
Before | After | Difference | |
---|---|---|---|
Dependency count | 192 | 192 | 0 |
Self size | 24 KB | 24 KB | 0 B |
Dependency size | 33.60 MB | 34.40 MB | 🚨 +802 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/react
Before | After | Difference | |
---|---|---|---|
Dependency count | 6 | 6 | 0 |
Self size | 932 KB | 1.73 MB | 🚨 +802 KB 🚨 |
Dependency size | 24 KB | 24 KB | 0 B |
Bundle Size Analyzer | Link | Link |
Co-authored-by: Kyle Gach <[email protected]>
Codemod: Add option to have relative path in csf factories codemod
Would love to get ahead of this for react native and try it out. Is this in a good place for me to try a canary version? |
Canary on this PR should be a good start! Lmk if you'd like to discuss |
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.
213 file(s) reviewed, 94 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -1,2 +1,9 @@ | |||
import { definePreview } from 'storybook/internal/preview-api'; |
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.
style: Consider using a more specific import path rather than 'storybook/internal/preview-api' to avoid relying on internal APIs
config?: Spec; | ||
|
||
/** | ||
* Options for the accessibility checks To learn more about the available options, |
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.
syntax: missing period after 'options' in comment
* | ||
* @see https://storybook.js.org/docs/writing-tests/accessibility-testing | ||
*/ | ||
a11y: { |
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.
logic: a11y property should be optional since it's a global configuration
a11y: { | |
a11y?: { |
@@ -1,2 +1,7 @@ | |||
// make it work with --isolatedModules | |||
export default {}; | |||
import { definePreview } from 'storybook/internal/preview-api'; |
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.
style: importing from 'storybook/internal/preview-api' suggests this is using an internal API - consider using a public import path instead
|
||
import * as addonAnnotations from './preview'; | ||
|
||
export default () => definePreview(addonAnnotations); |
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.
style: wrapping definePreview in a function call adds unnecessary indirection - consider exporting definePreview(addonAnnotations) directly
export default () => definePreview(addonAnnotations); | |
export default definePreview(addonAnnotations); |
Decorator: {args.decoratorArg} | ||
<Story args={{ decoratorArg: 0 }} /> |
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.
logic: Decorator modifies args but doesn't preserve original args when passing to Story component
Decorator: {args.decoratorArg} | |
<Story args={{ decoratorArg: 0 }} /> | |
Decorator: {args.decoratorArg} | |
<Story args={{ ...args, decoratorArg: 0 }} /> |
|
||
// optimization: stop HMR propagation in webpack |
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.
style: Duplicate comment line about HMR optimization
// optimization: stop HMR propagation in webpack |
// @ts-expect-error fix globalThis.Components type not existing later | ||
component: globalThis.Components.Button, |
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.
logic: globalThis.Components.Button needs proper TypeScript type definitions. This should be defined in a global.d.ts file or similar.
export function definePreview(preview: ReactPreview['input']) { | ||
return definePreviewBase({ | ||
...preview, | ||
addons: [reactAnnotations, reactDocsAnnotations, ...(preview.addons ?? [])], |
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.
style: Spreading preview.addons after the default addons means user addons could potentially override core functionality. Consider documenting this behavior.
args: Simplify< | ||
TArgs & Simplify<RemoveIndexSignature<DecoratorsArgs<ReactRenderer, Decorators>>> | ||
>; |
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.
style: Complex nested type transformations may impact type inference performance in large files. Consider splitting into intermediate types.
Closes #30166
What I did
This PR introduces a new way of writing stories using the CSF factories format, based on the RFC.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
DISCLAIMER:
This feature currently is only supported on react based frameworks (all of them).
VERY IMPORTANT to test: Monorepos, projects with multiple Storybooks which therefore would end up having multiple preview files.
The CSF Factory work involves many pieces:
The docs
There are explanations and instructions about the format in the docs. Please go through the documentation and see if it feels right: https://storybook.js.org/docs/api/csf/csf-factories (to be available once #30343 is merged and published)
Storybook itself
Storybook should work with CSF factories and with CSF123.
You are allowed to have a preview file using
definePreview
and have a story file not importing it, just using CSF3.If you are importing preview and creating a meta from it, you cannot mix story formats in that same file.
Storybook should also sync preview.js and main.js files when either running storybook dev or running storybook add.
say you have a main.js file like so:
it should add the following things to preview.js:
The codemods
The new format warrants many changes, in preview.js, main.js and story files. All of them are accounted for in the codemod. To run it, first upgrate Storybook to the most recent canary:
Then execute the codemod:
This should:
1 - Update your main.js file to use
defineMain
2 - Update your preview.js file to use
definePreview
3 - Ask about subpath imports
3.1 - If you say yes, it should update your package.json with the imports map
3.2 - If you say no, it should just continue
4 - Ask about stories glob and apply the codemod to all stories.
You are able to run the codemod multiple times. If you run it and select to use subpath imports, you can later rerun it to update all imports to relative if you prefer. Subpath imports will not work if the tsconfig file does not support it (you have to define module resolution to bundler).
Vitest integration
This PR adjusts the Vitest plugin transformation to support all CSF formats. It should work normally in codebases that have all formats.
Test-runner
The test-runner should behave correctly with CSF factories. All stories should be identified as tests.
Type safety
Typesafety for args should have feature parity with today. For example, required args might be passed both in meta and the story. And should warn if they are not present:
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH 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.ts
Make 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-30197-sha-de13df08
. Try it out in a new sandbox by runningnpx [email protected] sandbox
or in an existing project withnpx [email protected] upgrade
.More information
0.0.0-pr-30197-sha-de13df08
kasper/csf-factories
de13df08
1738833291
)To request a new release of this pull request, mention the
@storybookjs/core
team.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=30197
Greptile Summary
Based on the extensive changes in this PR, here's a concise summary of the CSF factories implementation:
Introduces CSF (Component Story Format) factories to provide type-safe story definitions and configuration in Storybook.
definePreview
anddefineMain
functions in/code/core/src/csf/csf-factories.ts
for type-safe configurationmeta.story()
pattern in/code/renderers/react/src/preview.tsx
for strongly-typed story definitions/code/lib/cli-storybook/src/codemod/csf-factories.ts
to convert existing stories/code/core/src/preview-api/modules/store/StoryStore.ts
/code/core/src/csf-tools/vitest-plugin/transformer.test.ts
for testing CSF factoriesThe implementation provides comprehensive type safety while maintaining backward compatibility with existing CSF formats. The changes are well-tested but require careful review around project annotation overrides and factory pattern adoption.