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

Core: Don't allow duplicate titles #7542

Merged
merged 4 commits into from
Jul 24, 2019
Merged

Core: Don't allow duplicate titles #7542

merged 4 commits into from
Jul 24, 2019

Conversation

shilman
Copy link
Member

@shilman shilman commented Jul 24, 2019

Issue: #7493

What I did

Restrict load function to unique "titles" per file & document

How to test

Edit a title in one file to match another, and load the storybook.

NOTE: it's currently possible to edit the title on HMR and sneak it through (it replaces the old one). But throws an error as intended on clean start. I think this is good enough.

@vercel
Copy link

vercel bot commented Jul 24, 2019

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

Latest deployment for this branch: https://monorepo-git-7493-duplicate-titles.storybook.now.sh

@shilman shilman added this to the 5.2.0 milestone Jul 24, 2019
@github-actions
Copy link
Contributor

github-actions bot commented Jul 24, 2019

Fails
🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against 2563c90

@tmeasday
Copy link
Member

Maybe warn and bail on that file rather than throw?

Otherwise LGTM

@shilman
Copy link
Member Author

shilman commented Jul 24, 2019

@tmeasday Throwing makes the error show up in the Storybook UI. It's a more extreme measure, but I think the warning will slip by a lot of people and lead to more support issues.

@@ -352,6 +355,11 @@ export default function start(render, { decorateStory } = {}) {
const { default: meta, ...exports } = fileExports;
const { title: kindName, parameters: params, decorators: decos, component } = meta;

if (loadedKinds.has(kindName)) {
throw new Error(`Duplicate kind: ${kindName} defined in multiple files`);
Copy link
Member

Choose a reason for hiding this comment

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

I'm good with the error being thrown, but it has to be an error a user can understand.

Please provide instructions to the user on what to do to resolve this.

@shilman shilman merged commit 29062d2 into next Jul 24, 2019
@shilman shilman deleted the 7493-duplicate-titles branch July 24, 2019 10:40
@obartra
Copy link

obartra commented Aug 1, 2019

Would you be open to combining stories with the same title (and only throw if the exports are duplicate)?

I was trying to have a story per file and I assumed that the title would handle the grouping. If I want to keep a story per file, is the recommended approach to have a separate file that re-exports the stories and sets the shared title?

@shilman
Copy link
Member Author

shilman commented Aug 1, 2019

@obartra that's a lot of files! why do you want to do that?

i made this change based on a HMR implementation detail, but this format is designed around "a file per component" and i don't expect that to change (tho i could be wrong!).

if you want a file per story, you'll need to use the workaround you suggested. tho i suspect you'd be better off adopting file-per-component since we're going to be building a lot of stuff around that grouping.

@obartra
Copy link

obartra commented Aug 1, 2019

Oh interesting, I guess leveraging knobs, I tend to write less stories but with more functionality on each. I usually end up with one story per component and a bunch of knobs to tweak the different props.

So trying out the new format I wanted to avoid having stories with a single nested story on each, something like this:

image

The re-exporting approach works great but I'm curious about the roadmap you mention. Is it publicly available? Happy to revisit my approach, particularly if it will play better with upcoming functionality

@shilman
Copy link
Member Author

shilman commented Aug 1, 2019

The most immediate example I can think of is SB docs:

https://medium.com/storybookjs/storybook-docs-sneak-peak-5be78445094a

We're transpiling MDX files into CSF in a webpack loader. If you write your stories in CSF you can also automatically migrate them into MDX should you choose. Might not be a big deal, but it's an example of something that comes "for free" with the format.

There's another big project by @ndelangen which will radically improve the loading speed of these files.

Re: knobs vs stories, we advocate to have more stories that represent the key states of your components for automated visual testing using something like Chromatic. (I'm biased since I helped build that, but would advocate this because I think it's a best practice and am building towards that regardless of self-interest).

I don't suppose there's a high "cost" to organizing things the way you're proposing and we'll certainly never do anything to intentionally make that difficult. But I also know that we're pretty bought into the "per-component" structure so that will always be supported well. Other structures? Your mileage may vary. (My $.02!)

@obartra
Copy link

obartra commented Aug 1, 2019

Thanks for sharing all that context, really appreciate the info. The new mdx format does look great.

Regarding Chromatic, do you get better performance if you combine more stories into larger ones? I'm doing visual diffing with a Cypress plugin and the page navigation cost became one of the drivers for consolidating stories.

@shilman
Copy link
Member Author

shilman commented Aug 1, 2019

@obartra we have an optimized rendering pipeline in chromatic which can test hundreds of stories per minute. you might gain a little speed by combining stories, but we try to make it fast so you don't have to.

@vinceumo
Copy link

vinceumo commented Sep 20, 2019

I'm trying to group components stories written with MDX. I can't use the title as Storybook doesn't allow duplicate titles. I have one MDX file per component, but I want to group them in the same folder. Anyway I could achieve that using MDX files?

Screenshot 2019-09-20 at 14 59 09

Currently need to have this structure instead

Screenshot 2019-09-20 at 16 35 21

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