-
-
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
WIP: CSF factories #30197
base: next
Are you sure you want to change the base?
WIP: CSF factories #30197
Changes from 13 commits
9415e47
66c8234
9fc6fbe
cdda2b5
5d05b3c
628e657
79107a5
60c6f26
c2988dc
6b0abf0
b4e6e25
c370fec
1d2ee2f
d04b258
410f988
25e9575
c061e82
3466750
84f2ae6
3753cde
5f7c6af
3eead18
5efce1e
dd5d1be
b70fdcc
e584c17
1f3600e
03a8b9c
b1bd157
ea5b8cb
03f8b11
07dcd78
e0ef30e
309a435
86fe532
49353cb
ec0c55f
ef92a43
745f1b5
30511e9
f59916d
eb50508
ddbe8b9
4127d87
740c149
5349a7d
f4e3a62
0c7bace
23bcf14
76b1b6c
0ae3643
4d2b108
53864bd
6d08dd1
dd7fd84
126cd95
19f4b0d
861625a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
{ 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 commentThe 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 |
||
context.skip(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ export async function generateModernIframeScriptCode(options: Options, projectRo | |
[], | ||
options | ||
); | ||
const previewAnnotationURLs = [...previewAnnotations, previewOrConfigFile] | ||
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 commentThe 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. |
||
|
||
|
@@ -23,14 +23,23 @@ export async function generateModernIframeScriptCode(options: Options, projectRo | |
// modules are provided, the rest are null. We can just re-import everything again in that case. | ||
const getPreviewAnnotationsFunction = ` | ||
const getProjectAnnotations = async (hmrPreviewAnnotationModules = []) => { | ||
const preview = await import('${previewFileUrl}'); | ||
const csfFactoryPreview = Object.values(preview).find(module => { | ||
return 'isCSFFactoryPreview' in module | ||
}); | ||
Comment on lines
+26
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
if (csfFactoryPreview) { | ||
return csfFactoryPreview.annotations; | ||
} | ||
|
||
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 commentThe 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. |
||
}`; | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-shadow | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,21 @@ import { global } from '@storybook/global'; | |
|
||
import { importFn } from '{{storiesFilename}}'; | ||
|
||
const getProjectAnnotations = () => composeConfigs(['{{previewAnnotations_requires}}']); | ||
const getProjectAnnotations = () => { | ||
const previewAnnotations = ['{{previewAnnotations_requires}}']; | ||
// the last one in this array is the user preview | ||
const preview = previewAnnotations[previewAnnotations.length - 1]; | ||
Comment on lines
+9
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Assuming the last preview is the user preview could be fragile if the array order changes. Consider adding a more explicit way to identify the user preview. |
||
|
||
const csfFactoryPreview = Object.values(preview).find((module) => { | ||
return 'isCSFFactoryPreview' in module; | ||
}); | ||
Comment on lines
+13
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Object.values() on preview could be undefined if preview is not an object. Add type checking or error handling. |
||
|
||
if (csfFactoryPreview) { | ||
return csfFactoryPreview.annotations; | ||
} | ||
|
||
return composeConfigs(previewAnnotations); | ||
}; | ||
|
||
const channel = createBrowserChannel({ page: 'preview' }); | ||
addons.setChannel(channel); | ||
|
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.