Skip to content
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

Merged

Conversation

joshwooding
Copy link
Contributor

Issue: #17548

What I did

Updated CSFFile parsing to check for storyName in CSF3 objects.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jun 11, 2022

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

@IanVS IanVS requested a review from shilman June 11, 2022 00:26
@IanVS
Copy link
Member

IanVS commented Jun 11, 2022

I tested this out in my own app, and it worked great.

@shilman shilman added bug patch:yes Bugfix & documentation PR that need to be picked to main branch csf3 labels Jun 11, 2022
Copy link
Member

@shilman shilman left a 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:

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:

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.

@IanVS
Copy link
Member

IanVS commented Jun 11, 2022

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 storyName because I figured out that the name wasn't what I thought it was (in CSF2), and it turned out to be the name of the function, which was minimized in production. If CSF3 does not support storyName, I think I can still make it work, by checking whether the story is a function or an object, and using either storyName or name accordingly. I didn't realize this was a change, it would be great to have some information about this kind of breaking-ish change in https://github.com/storybookjs/storybook/blob/next/MIGRATION.md, and maybe some CSF3-specific docs as well (maybe they exist and I haven't found them).

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 name instead of storyName.

@IanVS
Copy link
Member

IanVS commented Jun 11, 2022

Oh also, I'm not sure why, but I have this type definition, which also made me think that storyName was fine to use:

export declare type StoryAnnotations<TFramework extends AnyFramework = AnyFramework, TArgs = Args> = BaseAnnotations<TFramework, TArgs> & {
    name?: StoryName;
    storyName?: StoryName;
    play?: PlayFunction<TFramework, TArgs>;
    story?: Omit<StoryAnnotations<TFramework, TArgs>, 'story'>;
};

StoryAnnotations is what is used by StoryObj, which I thought was CSF3. This is in @storybook/csf@0.0.2--canary.4566f4d.1, which I'm not sure what that actually corresponds to, or if it's the latest, or if something needs to be updated.

@shilman
Copy link
Member

shilman commented Jun 11, 2022

@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.

@joshwooding joshwooding force-pushed the fix-csf3-storyName-storyStoreV7 branch from 3bba2d7 to 084043e Compare June 11, 2022 23:41
Copy link
Member

@shilman shilman left a 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 🙏

@shilman shilman changed the title Fix storyName not working with CSF3 and storyStoreV7 enabled Story index: Warn on storyName in CSF3 exports Jun 12, 2022
@shilman shilman changed the title Story index: Warn on storyName in CSF3 exports Story index: Warn on storyName in CSF3 exports Jun 12, 2022
@shilman shilman merged commit c803e54 into storybookjs:next Jun 12, 2022
@joshwooding joshwooding deleted the fix-csf3-storyName-storyStoreV7 branch June 12, 2022 00:55
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 12, 2022
shilman added a commit that referenced this pull request Jun 12, 2022
…oreV7

Story index: Warn on `storyName` in CSF3 exports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug csf3 patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch story index
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants