-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Define pattern for story reuse in CSF3 #15954
Comments
Hmm, yeah this is a downside of the CSFv3 approach. We could have a version of import { prepareStoryForReuse } from '@storybook/store';
export const OneItemCallingRenderFunction = {
args: {
listofItems: 2
},
render: args => (
<List {...args}>{prepareStoryForReuse(Unchecked)}</List>
)
}; |
I'm good with what you or @shilman decide on this. That construct you've mentioned doesn't sound bad at first glance and possibly could be used. But like I said I'll leave it up to both of you on how to proceed and let me know and I'll adjust the documentation. Thank you for the quick response on this! |
@shilman @tmeasday this is a quite tricky situation. In CSF2 things were simple in terms of portability, but now with CSF3 the Meta will always be required in order to compose stories, given that the render function is either It's also tricky to not include decorators when composing the story, because you're not always sure the story will work without its decorators. For simple use cases it would definitely work, but an example of a more complex scenario could be a PageStory that reuses a ProductsListStory that mocks stuff based on a decorator set only to that story and not globally. Sometimes people also use a combination of args + decorators, so it's difficult to get all cases right. I think we need to put some more thought into this. Let me know if you'd like to setup a call or something, might be easier to discuss things |
Just a random idea, I don't know if it makes sense: If we were to expose a CSF object that already processes stuff using a function like const meta = {
title: 'Example/Button',
component: Button,
args: {
primary: true,
label: "Hello"
},
decorators: [withTheme]
};
export default meta;
export const Primary = {};
export const Secondary = { args: { primary: false } };
// automatically not treated as a story
export const CSF = {
Primary: composeStories(meta, Primary),
Secondary: composeStories(meta, Secondary),
};
// another flavor of the example above
export const CSF = composeStories({ default: meta, Primary, Secondary }); and somewhere else it could be reused like: import { CSF } from './Button.stories';
const { Primary, Secondary } = CSF;
// This would always work
export const Playground = () => (
<div>
<Primary somePropToOverride />
<Secondary />
</div>
) Edit: I tested something else which happened to work, however this would make a complicated typescript situation. In testing-react's export const CSF = composeStories(module.exports); |
@yannbf thanks for your thoughts.
I think this is a key point. The thing is that it is totally unclear which decorators are needed and which aren't in both: a) this compositional situation Decorators come in a bunch of different varieties and some apply to (a), some to (b) and some neither. We could thrash this out but I don't think there's necessarily going to be an API way to determine that, it might ultimately come down to the user to decide. I think this is only going to be a bigger and more obvious issue as we start pushing connected components and pages into SB further, where decorates as data providers are going to be key.
I feel like this muddies the water though. Folks will be rightfully confused about what's what I would have thought (which one is the thing that gets executed? Do I need the CSF bit? etc). One direction we could go in would be to really commit to the idea of "the thing that is exported by the CSF file is executable" -- and CSFv3 instead could be: export const Primary = composeStories(meta, {});
export const Secondary = composeStories(meta, { args: { primary: false } }); However, I think that actually doesn't solve the problem, because as I mentioned above, there is no "executable version" of the story that will work in every possible use-case (I don't think anyway). Some case require one set of decorators, some another. But it is definitely true that when we introduced args and args inheritance in CSFv2, we took a big step away from "exported stories are renderable". It was true however that CSFv1 stories weren't always renderable either (see decorators, again). But they were in the bulk of cases. So this is a problem that has been bubbling away for a while.
Absolutely agree. We should probably try and figure it out before 6.4 or at least before we push CSFv3 as the recommended feature. As an aside I know @jonniebigodes who brought up this issue is a massive advocate of CSFv3 and thinks it makes for a much better DX. My current feeling is the DX benefits of CSFv3 out weigh the downsides of making exports less renderable. But I want to get to the bottom of it for sure! |
Going to note down some quotes from #12654 here to give more context to the problem: This was and currently is the downside of CSFv3.
We arrived at a probable solution in that issue:
But I think there's one deeper problem that is happening even with CSFv2:
@tmeasday had this idea in one of the other issues: I think to solve this reuse issue, let's define how CSFv2 currently works: (Feel free to let me know if I am wrong and I will update this comment) Inside another storyimport React from 'react';
import List from './List';
//👇 Instead of importing ListItem, we import the stories
import { Unchecked } from './ListItem.stories';
export const OneItem = (args) => (
<List {...args}>
<Unchecked {...Unchecked.args} />
</List>
); What decorators does the
Inside testing (variation 1)import React from 'react';
import { render, screen } from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';
//👇 Imports a specific story for the test
import { Primary } from './Button.stories';
it('renders the button in the primary state', () => {
render(<Primary {...Primary.args} />);
expect(screen.getByRole('button')).toHaveTextContent('Primary');
}); What decorators does the
Inside jest testing (variation 2)import { render, screen } from '@testing-library/react';
import { composeStories } from '@storybook/testing-react';
//👇 Imports all stories for the test
import * as stories from './Button.stories';
// Every component that is returned maps 1:1 with the stories, but they already contain all decorators from story level, meta level and global level.
const { Primary, Secondary } = composeStories(stories);
test('renders primary button with default args', () => {
render(<Primary />);
const buttonElement = screen.getByText(/Text coming from args in stories file!/i);
expect(buttonElement).not.toBeNull();
}); What decorators does the
|
(note comment is now updated)
In these two usages, no decorators would apply.
[1] As an aside we renamed these to "project" decorators internally to remove confusion between the concepts of "globals" and "global annotations". Thanks for bringing this up again. @yannbf and I discussed this recently and covered the following:
Let's talk about the different decorators and when you would want to apply them to the two main use cases (story composition and external re-use):
[2] My assumption would be the test script wouldn't need the element to be "correct" HTML. I'm sure there are other categories I haven't thought of, let's discuss and I can update the table. In any case, my conclusion so far is: (1) You don't want any decorators for the Story Composition use case. For (2) we were talking about giving the user (and the addon author?) a way of marking decorators as "required" or "data-providing" or something, and distinguishing between the two when preparing a story for external use. It doesn't seem like a great solution though. |
I am tempted to say that people would like to reduce duplication and thus reuse the loaded data too, but I can also see cases where the wrapping story would 100% provide the data. Thus, I would agree with the following:
I can imagine some tests that interact with that outer html and component. And wouldn't visual tests (like chromatic) depend on that visual decorators? |
Hmm, maybe? Perhaps you can show an example? I'm wondering if that would be testing the right thing ;)
I think a certain type of visual test would, for sure. I don't anticipate Chromatic ever rendering the story directly, rather than inside the Storybook, but I guess if we made this solution solid enough we could consider it 🤔 |
Any word on this? it's been about 1.5 years. I'm trying to upgrade my project to csf3, but this is blocking the upgrade by breaking any story that is re-used elsewhere. |
So I think the challenge here (as pointed out by @yannbf) is composing down The decorator discussion, although useful was a side-track, given we concluded we don't need them! @yannbf maybe a simpler approach would be: import { prepareForReuse } from '@storybook/preview-api';
import * as ListItemStories from './ListItem.stories';
export const OneItemCallingRenderFunction = {
args: {
listofItems: 2
},
render: (args) => (
<List {...args}>{prepareStoryForReuse(ListItemStories.Unchecked, ListItemStories.default)}</List>
)
};
// Or maybe cleaner:
const { Unchecked } = prepareForReuse(ListItemStories);
// then:
<List {...args}><Unchecked /></List> The |
@tmeasday I would be really interested to know how your idea should work. I tried to implement something like this but there are more problems:
My current best approach (for React + Typescript) looks like this: export const renderStory = <M,>(
meta: M,
story: StoryObj<M>,
additionalArgs: Partial<StoryObj<M>["args"]> & { key?: React.Key } = {}
) => {
if (!story.render) {
const Component = (meta as Meta).component ?? "div";
return <Component {...story.args} {...additionalArgs} />;
}
// Render needs the story context as second parameter but this doesn't exist in that case.
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
return story.render?.({ ...story.args, ...additionalArgs });
}; usage: import * as IconStory from "../Icon/Icon.story.tsx";
...
export const ButtonWithIcon: Story = {
args:
icon: renderStory(IconStory.default /* forward meta manually /*, IconStory.Base)
}
}; This is a huge missing feature and is currently blocking us on multiple large codebases. |
I had a thought, maybe
Right well I think you'd have to construct a context. I guess the keys you'd need to put on the context would depend on what is used in the story's render function. Ideally it'd have them all but that's a bit complicated: storybook/code/lib/preview-api/src/modules/store/StoryStore.ts Lines 290 to 295 in 1dc1ed5
storybook/code/lib/preview-api/src/modules/store/csf/prepareStory.ts Lines 105 to 116 in 1dc1ed5
But I'm guessing a typical
I don't think it's super helpful to use that kind of language. @jonniebigodes who maintains our docs opened this ticket after all. |
We have a reusable pattern that we leverage for Attempted migration to s7 stopped it in its tracks: export const prepareContainerStories = (
Component: ComponentType,
mockHandlers: MockRestFunction[],
response?: DefaultBodyType,
) => {
const Template: Template = () => <Component />;
const Default = Template.bind({});
Default.parameters = {
msw: {
handlers: mockHandlers.map((handler) =>
handler(200, false, undefined, response),
),
},
};
Default.decorators = [
(Story: Story) => {
useEffect(() => {
return () => window.location.reload();
}, []);
return <Story />;
},
];
const Loading = Template.bind({});
Loading.parameters = {
msw: { handlers: mockHandlers.map((handler) => handler(undefined, true)) },
};
Loading.decorators = [
(Story: Story) => {
useEffect(() => {
return () => window.location.reload();
}, []);
return <Story />;
},
];
const Error = Template.bind({});
Error.parameters = {
msw: {
handlers: mockHandlers.map((handler) => handler()),
},
};
Error.decorators = [
(Story: Story) => {
useEffect(() => {
return () => window.location.reload();
}, []);
return <Story />;
},
];
return {
Default,
Error,
Loading,
};
}; Feels like a step backwards to go to Storybook 7/csf3 when you have so much boilerplate and set up to replicate. Loved this reusable pattern possible before and not seeing a way to do this now, so we wait to migrate. |
@tmeasday surely its time to change the docs by now? I ended up here because I did what the docs say, and it does not work. It doesn't look like this is an easy problem to solve. If this is going to take another 2 years, how can you justify leaving incorrect information in the docs? |
Hey there @kwickramasekara @ryanwolhuter this is a tough problem because it would need to work for all renderers. In the meantime, it should work for React specifically, in the following scenarios:
import { Unchecked } from './Checklist.stories'
// omitting default export for brevity
export const OneItemCallingRenderFunction = {
args: {
listofItems: 2
},
render: args => (
<List {...args}>{Unchecked.render({ ...Unchecked.args })}</List>
)
};
import { composeStories } from '@storybook/react'
import * as stories from './Checklist.stories'
const { Unchecked } = composeStories(stories)
// omitting default export for brevity
export const OneItemCallingRenderFunction = {
args: {
listofItems: 2
},
render: args => (
<List {...args}><Unchecked/></List>
)
}; |
@yannbf I agree that it is a hard problem. thank you for sharing the existing workaround, I will keep it in mind. however the point I am making is simply that this is what the docs currently say: import type { Meta, StoryObj } from '@storybook/react';
import { List } from './List';
//👇 Instead of importing ListItem, we import the stories
import { Unchecked } from './ListItem.stories';
export const meta: Meta<typeof List> = {
/* 👇 The title prop is optional.
* See https://storybook.js.org/docs/configure/#configure-story-loading
* to learn how to generate automatic titles
*/
title: 'List',
component: List,
};
export default meta;
type Story = StoryObj<typeof List>;
export const OneItem: Story = {
render: (args) => (
<List {...args}>
<Unchecked {...Unchecked.args} />
</List>
),
}; And it is clear to everyone in this thread that doing this does not work. Therefore, the docs are setting users up to fail by giving them wrong instructions. Either this section should be removed from the docs, so that people aren't lead to believe that they can do something that they in fact cannot, or else the docs should be changed to make it clear that this issue exists and point users to the workaround you posted. |
@jonniebigodes - @ryanwolhuter is right - at the least we should mention the above will only work for CSF2-. I think we could mention there are various renderer-specific approaches for CSF3 and link to @yannbf's comment or something similar? |
Hey everyone, Apologies for the auto-close. Before sharing this comment, I was waiting for our patching process to complete and thus make the doc updates available: https://storybook.js.org/docs/writing-stories/stories-for-multiple-components We had hoped that the work done for portable stories would enable a way forward on this issue. Unfortunately, there are just too many caveats and gotchas for that to be a recommendation we felt like we could make. With that possibility exhausted, we've come to the conclusion that re-using a full story within another is not something accommodated by CSF (as Tom pointed out, above, the root issue was introduced with CSF 1). So, we decided to remove the guidance around doing so to avoid further confusion and frustration. Instead, we suggest re-using relevant parts of the story definition (e.g. just the We realize this isn't a satisfying conclusion for many of you. When looking at the future of CSF, please know that this use case is one we'll be weighing. |
@yannbf |
While going through the changes required in terms of snippets for CSF3 and React. I came across this pattern currently documented with CSF 2.0 and Storybook.
Looking at this through the lens of Storybook 6.4 and CSF 3.0 I thought that the same pattern could be applied. But it seems that's not the case. Using said pattern with CSF 3.0 leads to some issues. As a testing ground, I've created a reproduction here so that it can be looked at.
Looking at the
stories/List.stories.jsx
the story namedOneItem
doesn't actually work. Throws an error. As an alternative I've created the following story:This actually works and it's rendered fine with Storybook. But this introduces some issues, as we cannot guarantee that the user has a
render
function defined in his/her stories.@tmeasday I know that you're reworking part of Storybook that affects this particular section of the documentation. If you wouldn't mind following up with me on this once it's implemented I'd appreciate it.
One thing I'd like to point out that it's a bit similar to this is the usage of children as args as documented here. Probably it will require a similar approach as for this case.
@shilman as per our conversation a bit earlier, here's the issue.
The text was updated successfully, but these errors were encountered: