-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Story index: Warn on storyName
in CSF3 exports
#18464
Story index: Warn on storyName
in CSF3 exports
#18464
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 084043e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
I tested this out in my own app, and it worked great. |
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.
Hi @joshwooding thanks for the fix! Code & test both look good. 👍
However, I'd like to discuss the change before merging.
The reason storyName
is not supported is because the annotation has been renamed to name
in CSF.
It would have been called name
in the first place, except that in CSF2 the following errors in in JS due to name
being a reserved property:
export const Foo = (args) => { ... };
Foo.name = 'FooBar'; // error
You can see that the CSF parser supports storyName
for CSF2 here:
storybook/lib/csf-tools/src/CsfFile.test.ts
Lines 141 to 157 in 3bba2d7
it('storyName annotation', () => { | |
expect( | |
parse( | |
dedent` | |
export default { title: 'foo/bar' }; | |
export const A = () => {}; | |
A.storyName = 'Some story'; | |
` | |
) | |
).toMatchInlineSnapshot(` | |
meta: | |
title: foo/bar | |
stories: | |
- id: foo-bar--a | |
name: Some story | |
`); | |
}); |
And you can also see that the codemod from CSF2 to CSF3 updates it here:
storybook/lib/codemod/src/transforms/__tests__/csf-2-to-3.test.ts
Lines 34 to 56 in 3bba2d7
it('should move annotations into story objects', () => { | |
expect( | |
jsTransform(dedent` | |
export default { title: 'Cat' }; | |
export const A = () => <Cat />; | |
A.storyName = 'foo'; | |
A.parameters = { bar: 2 }; | |
A.play = () => {}; | |
`) | |
).toMatchInlineSnapshot(` | |
export default { | |
title: 'Cat', | |
}; | |
export const A = { | |
render: () => <Cat />, | |
name: 'foo', | |
parameters: { | |
bar: 2, | |
}, | |
play: () => {}, | |
}; | |
`); | |
}); |
In the interest of keeping CSF3, I'd rather detect storyName
and throw an informative error asking the user to rename it to name
.
What do you think? I don't feel too strongly about this.
For my own use case, I have a mix of CSF2 and (mostly) CSF3, and I have a helper component which needs to be able to read the name of the story. I am using It would also be great if we could somehow find some folks who are willing to help step in and triage issues like #17548, which it sounds like could have been closed out a long time ago by saying to use |
Oh also, I'm not sure why, but I have this type definition, which also made me think that storyName was fine to use:
|
@jonniebigodes has a PR in progress with CSF3 docs. Getting people upgraded smoothly will be a focus of SB7 when CSF3 will become the recommended syntax. As for the CSF types, that looks like an incorrect type. I'll see if I can get it worked out with @tmeasday and clean up the versions. Re: triage, I will try to get back on it as we grow the dev team. I was doing triage every day for the past few years, but at some point late last year I got overwhelmed and haven't been able to get back on top of it since then. |
3bba2d7
to
084043e
Compare
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.
LGTM. Thanks for your patience on this 🙏
storyName
in CSF3 exports
…oreV7 Story index: Warn on `storyName` in CSF3 exports
Issue: #17548
What I did
Updated CSFFile parsing to check for storyName in CSF3 objects.
How to test
If your answer is yes to any of these, please make sure to include it in your PR.