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

CLI: Avoid id changes after storiesof-to-csf migration #8856

Merged
merged 6 commits into from
Nov 20, 2019
Merged

Conversation

Hypnosphi
Copy link
Member

Issue: sometimes running storiesof-to-csf codemod leads to story id changes

What I did

  1. Replaced camelCase with PascalCase in export names which rules out clashes with reserved keywords
  2. Replaced Story suffix with _ prefix which gets stripped during conversion to id

@vercel
Copy link

vercel bot commented Nov 18, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/5c8fki44v
🌍 Preview: https://monorepo-git-avoid-id-changes.storybook.now.sh

@Hypnosphi Hypnosphi requested a review from shilman November 18, 2019 00:18
if (isReserved(key)) {
key = `${key}Story`;
}
let key = upperFirst(camelCase(name));
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to use pipeline operator here, but I'm not sure whether it's OK with other contributors:

let key = name |> camelCase |> upperFirst

Copy link
Member

Choose a reason for hiding this comment

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

🤔 stage 1 ....

Copy link
Member

Choose a reason for hiding this comment

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

Let's not use language features that typescript doesn't support?

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.

This makes sense & is definitely an improvement over the existing behavior.

My one concern is about PascalCase, since I don't know whether we should encourage that convention for story exports. What do you think about changing it to _${camelCase(name)} in those cases?

Curious to hear what you and others think.

@atanasster
Copy link
Member

Just my 0.02c camelCase seems more natural in javascript projects /coming from a 20+ years Pascal guy :)/

@tmeasday
Copy link
Member

I discussed this a little with @shilman and I think I agree that the "prefix reserved words with _" solution is probably a better way to achieve the same goal. Here's my thought process:

  1. If we update the codemod to PascalCase but not our examples, people using the codemod are going to be put off: "this doesn't look like the docs, did it break or something?". I don't love that.
  2. If we update our examples, the first thing a new user to storybook is going to see is something like:
export Welcome = () => <Welcome />;

I guess technically there's nothing wrong with a pascal-cased export but I reckon if I was a new user it would put me off.

@Hypnosphi
Copy link
Member Author

camelCase seems more natural in javascript projects

But using PascalCase for components in JSX-based frameworks and libraries like React is quite a common practice. And the stories look pretty much like components (even if technically they aren't), and we even allow React hooks in them. So maybe PascalCasing them by default is a good thing? Among other things, it would:

  1. Remove the need to suppress react-hooks/rules-of-hooks ESLint rule when using React hooks. Right now it complains because it doesn't treat a camelCased function as a component
  2. Make it easier to reuse stories inside other stories. You could do just <MyStory /> without reassignments like const MyStory = myStory

If we update the codemod to PascalCase but not our examples, people using the codemod are going to be put off: "this doesn't look like the docs, did it break or something?". I don't love that.

It makes sense to me to update the examples as well. I can do it on approval

If we update our examples, the first thing a new user to storybook is going to see is something like:

export Welcome = () => <Welcome />;

Not really. What they actually would see in case of official-storybook is

export const ToStorybook = () => <Welcome showApp={linkTo('Button')} />;

@tmeasday
Copy link
Member

That's a good point @Hypnosphi -- I think I am convinced, although it seems an important change.

@shilman
Copy link
Member

shilman commented Nov 18, 2019

That might be the case for React, but not for any of our other frameworks where we're returning JS objects... My gut still says _camelCase is the way to go. That said, I'm not married to that, so if you feel strongly about PascalCase then I reluctantly approve. 😄

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Nov 18, 2019

OK. Given that the benefits for React (and maybe other JSX-based) are practical while the downsides for other frameworks are purely stylistical, let me migrate examples and CLI templates then

@shilman shilman added this to the 5.3.0 milestone Nov 20, 2019
@shilman shilman changed the title storiesof-to-csf: avoid id changes after migration CLI: Avoid id changes after storiesof-to-csf migration Nov 20, 2019
@shilman shilman merged commit e201a95 into next Nov 20, 2019
@shilman shilman deleted the avoid-id-changes branch November 20, 2019 00:31
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.

5 participants