-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Add default export as function annotation from core addons #30376
base: kasper/csf-factories
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit e71bade.
☁️ Nx Cloud last updated this comment at |
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.
31 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
|
||
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: Consider adding a type annotation for the return value to ensure type safety
export { PARAM_KEY } from './constants'; | ||
|
||
export default () => definePreview({}); |
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: empty object passed to definePreview() suggests this addon has no preview annotations, which seems incorrect for a controls addon that likely needs to register parameters or decorators
@@ -1 +1,5 @@ | |||
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 the full path '@storybook/internal/preview-api' instead of 'storybook/internal/preview-api' for consistency with other imports in the codebase
// perhaps csf types need to be updated? StepRunner expects Promise<void> and not Promise<void> | void | ||
).step as StepRunner; |
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: The type assertion may hide a real type mismatch between StepRunner and the instrumented step function. Consider fixing the underlying type definitions instead of using a type assertion.
import { PARAM_KEY } from './constants'; | ||
import { withMeasure } from './withMeasure'; | ||
|
||
export const decorators: Addon_DecoratorFunction[] = [withMeasure]; | ||
export const decorators = [withMeasure]; |
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: Removing the explicit Addon_DecoratorFunction type could make it harder to catch type errors if withMeasure's type signature changes in the future
export * from './defaults'; | ||
export type * from './types'; | ||
|
||
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: Consider adding a return type annotation to the default export function for better type safety
if (!data.isCoreAddon) { | ||
data.importPath = `@storybook/${addon}/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.
style: Non-core addon path assumes @storybook namespace which may not always be true. Consider using the original addon string instead of hardcoding the namespace.
(getAddonAnnotations as Mock).mockImplementation(() => { | ||
return { | ||
importName: 'addonA11yAnnotations', | ||
importPath: '@storybook/addon-a11y', | ||
isCoreAddon: true, | ||
}; | ||
}); |
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 mock implementation returns a different importPath compared to the first test case ('@storybook/addon-a11y' vs '@storybook/addon-a11y/preview'). Consider adding a test case that verifies the behavior when both paths are used in the same preview config
|
||
import { PARAM_KEY } from './constants'; | ||
import { addOutlineStyles, clearStyles } from './helpers'; | ||
import outlineCSS from './outlineCSS'; | ||
|
||
export const withOutline = (StoryFn: StoryFunction<Renderer>, context: StoryContext<Renderer>) => { | ||
export const withOutline: DecoratorFunction = (StoryFn, context) => { |
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 DecoratorFunction
type should be properly constrained with a Renderer type parameter to maintain type safety
|
||
import { PARAM_KEY } from './constants'; | ||
import { addOutlineStyles, clearStyles } from './helpers'; | ||
import outlineCSS from './outlineCSS'; | ||
|
||
export const withOutline = (StoryFn: StoryFunction<Renderer>, context: StoryContext<Renderer>) => { | ||
export const withOutline: DecoratorFunction = (StoryFn, context) => { | ||
const { globals } = context; | ||
const isActive = [true, 'true'].includes(globals[PARAM_KEY]); |
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 Boolean()
instead of array includes for cleaner type coercion
export default composeConfigs([actions, docs, backgrounds, viewport, measure, outline, highlight]); | ||
export default composeConfigs([ | ||
actionsAddon(), | ||
docsAddon(), | ||
backgroundsAddon(), | ||
viewportAddon(), | ||
measureAddon(), | ||
outlineAddon(), | ||
highlightAddon(), | ||
]); |
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.
I think this file can be moved into index.ts
, as it didn't exist before.
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.
The index.ts
file is actually a node preset file:
https://github.com/storybookjs/storybook/blob/next/code/addons/essentials/src/index.ts
Not sure what to do here!
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
Before | After | Difference | |
---|---|---|---|
Dependency count | 4 | 4 | 0 |
Self size | 27 KB | 53 KB | 🚨 +26 KB 🚨 |
Dependency size | 99 KB | 99 KB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/addon-essentials
Before | After | Difference | |
---|---|---|---|
Dependency count | 36 | 36 | 0 |
Self size | 15 KB | 15 KB | 0 B |
Dependency size | 13.87 MB | 13.97 MB | 🚨 +101 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 | 68 KB | 🚨 +36 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 | 50 KB | 🚨 +27 KB 🚨 |
Dependency size | 32 KB | 32 KB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/nextjs
Before | After | Difference | |
---|---|---|---|
Dependency count | 592 | 592 | 0 |
Self size | 474 KB | 474 KB | 0 B |
Dependency size | 84.21 MB | 84.23 MB | 🚨 +17 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 323 | 323 | 0 |
Self size | 8 KB | 8 KB | 0 B |
Dependency size | 43.26 MB | 43.27 MB | 🚨 +14 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/vue3-webpack5
Before | After | Difference | |
---|---|---|---|
Dependency count | 495 | 495 | 0 |
Self size | 7 KB | 7 KB | 0 B |
Dependency size | 55.65 MB | 55.67 MB | 🚨 +17 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/cli
Before | After | Difference | |
---|---|---|---|
Dependency count | 401 | 401 | 0 |
Self size | 529 KB | 529 KB | 0 B |
Dependency size | 75.61 MB | 75.62 MB | 🚨 +18 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/codemod
Before | After | Difference | |
---|---|---|---|
Dependency count | 277 | 277 | 0 |
Self size | 612 KB | 612 KB | 0 B |
Dependency size | 65.39 MB | 65.41 MB | 🚨 +18 KB 🚨 |
Bundle Size Analyzer | Link | Link |
Tnx broo |
Closes #30242
What I did
This PR introduces the concept of
definePreview
in preview-api, to be used by addon authors. It also uses that concept as part of the core addons. It updates the syncing logic to take into account so that core addons will be added like this in csf factories: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: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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/core
team here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>
Greptile Summary
Based on the provided information, I'll create a summary of the key changes in this PR:
Introduces
definePreview
function for standardizing addon configuration in Storybook, with significant changes to core addons and their integration patterns.definePreview
function incode/core/src/preview-api/modules/addons/definePreview.ts
for type-safe addon configurationsync-main-preview-addons.ts
to handle different import patterns for core vs non-core addonsDecoratorFunction
type instead of explicitRenderer
andStoryContext
typesThe changes improve type safety and standardize how addons are registered and configured in Storybook, particularly focusing on core addons that will now be added using function calls rather than direct imports.