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

CSF: Transform CSF named exports w/ makeDisplayName #7878

Merged
merged 6 commits into from
Aug 27, 2019

Conversation

shilman
Copy link
Member

@shilman shilman commented Aug 27, 2019

Issue: #7599

What I did

  • Add options.makeDisplayName parameter
  • Default to lodash.startCase
  • Apply to CSF named exports only
  • Update the MDX compiler to always preserve the name

Examples (input => output):

      const testCases = {
        name: 'Name',
        someName: 'Some Name',
        someNAME: 'Some NAME',
        some_custom_NAME: 'Some Custom NAME',
      };

This dramatically improves the ergonomics of CSF (thanks @kevinSuttle!!) but is disruptive to anybody who's already using CSF. Moreover it will be expensive to change in the future so I really want to get this right. Feedback wanted.

Other options I considered:

  • Make this opt-in
  • Apply this to all story names and not just CSF

How to test

  • E2E: Source.stories.tsx change and output in official-storybook
  • Unit: yarn test --testPathPattern client_api.test.ts

@vercel
Copy link

vercel bot commented Aug 27, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@shilman shilman added this to the 5.2.0 milestone Aug 27, 2019
@@ -82,6 +83,8 @@ const withSubscriptionTracking = (storyFn: StoryFn) => {
return result;
};

export const defaultDisplayName = key => startCase(key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/client_api.ts(86,35): error TS7006: Parameter 'key' implicitly has an 'any' type.

@shilman
Copy link
Member Author

shilman commented Aug 27, 2019

UPDATE: @ndelangen has suggested an alternative where instead of changing the name that's passed to the story_store, we create a displayName parameter that only affects the display. The benefits of this approach is that the Story IDs will be more stable/predictable and won't be affected by code transformations. I'm going to try this out. It's a bigger change than this one, but it's important that we get this right.

@Hypnosphi
Copy link
Member

Should we really mark it as a breaking change if it only affects functionality from beta?

@shilman
Copy link
Member Author

shilman commented Aug 27, 2019

@Hypnosphi Good point. I've created a new label, BREAKING PRERELEASE to document this

@@ -123,6 +126,10 @@ export default class ClientApi {
this._globalParameters.options
);

getDisplayName = () =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns a function?

I think this method name is misleading

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makeGetDisplayName

@shilman shilman changed the title CSF: Transform CSF named exports with a displayName option CSF: Transform CSF named exports with makeDisplayName option Aug 27, 2019
@shilman shilman changed the title CSF: Transform CSF named exports with makeDisplayName option CSF: Transform CSF named exports w/ makeDisplayName option Aug 27, 2019
@shilman shilman changed the title CSF: Transform CSF named exports w/ makeDisplayName option CSF: Transform CSF named exports w/ makeDisplayName Aug 27, 2019
@kevinSuttle
Copy link
Contributor

Thanks @shilman! Apologies for not getting back to the PR. 🙇

@shilman
Copy link
Member Author

shilman commented Aug 27, 2019

@kevinSuttle My pleasure. Thanks for conceiving this feature -- it's huge for CSF ergonomics

@Hypnosphi
Copy link
Member

Hypnosphi commented Sep 17, 2019

Story IDs {...} won't be affected by code transformations

Unfortunately, this is what actually happens now in a couple of corner cases like following:

storiesOf('MyStories').add('default', () => <MyComponent />)

storiesof-to-csf cannot use reserved word default as an export name, so it uses defaultStory:

export default {
  title: 'MyStories',
};

export const defaultStory = () => <MyComponent />;

defaultStory.story = {
  name: 'default',
};

Oddly enough, story.name isn't used as actual story name, but "Default Story" is used instead. It also changes id from /story/my-stories--default to /story/my-stories--default-story

It's even worse in case of collision, when story0 is used as function name, and "Story 0" as story name. #8106 makes it fooStory / "Foo Story" in case of foo name collision

@shilman
Copy link
Member Author

shilman commented Sep 17, 2019

You can currently control the ID and display name independently, though there is no way to get a reserved keyword as the ID. do you have any suggestions for how to do it differently? We might be able to change in 6.0

@Hypnosphi
Copy link
Member

Hypnosphi commented Sep 17, 2019

You can currently control the ID and display name independently

Honestly, it brings more confusion than value. Here's my suggestion:
– Add consistentNames option in 5.3. It will use exportedFunction.story.name as storyObject.name.
– Make it the default and only behavior in 6.0

@shilman
Copy link
Member Author

shilman commented Sep 17, 2019

@ndelangen wants the ID to be controlled by the named export for monoconfig. Won't make any changes until we make sure they are compatible with what he needs

@Hypnosphi
Copy link
Member

Another example of ID change after applying transform: #8000 (comment)

@ndelangen
Copy link
Member

ndelangen commented Sep 25, 2019

– Add consistentNames option in 5.3. It will use exportedFunction.story.name as storyObject.name.
– Make it the default and only behavior in 6.0

I'm fine with that, monoconfig won't be happening before 6.0 anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants