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

feat: add defineMeta #25069

Closed
wants to merge 1 commit into from
Closed

Conversation

AriPerkkio
Copy link
Contributor

Closes #18228

What I did

Adds new helper utility defineMeta.

import { defineMeta } from '@storybook/react';
import { Button } from './Button';

// Type-safety out-of-the-box without writing any Typescript!
export default defineMeta({
  title: 'Example/Button',
  component: Button,
});

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

  1. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. cd sandbox/react-vite-default-ts
  3. Open sandbox/react-vite-default-ts/src/stories/Button.stories.ts in your code editor
  4. Apply following changes:
-  3 | import type { Meta, StoryObj } from '@storybook/react';
+  3 | import { type  StoryObj, defineMeta } from '@storybook/react';
   4 |
   5 | import { Button } from './Button';
   6 |
   7 | // More on how to set up stories at: https://storybook.js.org/docs/writing-stories#default-export
-  8 | const meta = {
+  8 | const meta = defineMeta({
   9 |   title: 'Example/Button',
     | ...
- 21 | } satisfies Meta<typeof Button>;
+ 21 | });
  1. yarn storybook and verify Button stories work in browser
  2. Apply following changes to same file:
-  8 | const meta = defineMeta({
+  8 | export default defineMeta({
     | ...
- 22 | type Story = StoryObj<typeof meta>;
+ 22 | type Story = StoryObj;
  1. Verify Button stories work in browser
  2. Intentionally break the implementation, e.g. csf-tools/src/CsfFile.ts and see how stories no longer work.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Should I add documentation to docs/writing-stories/typescript.md? Maybe under the ## Using 'satisfies' for better type safety?

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@valentinpalkovic
Copy link
Contributor

Thank you for your contribution to Storybook with the defineMeta utility.

Before moving forward, there are several considerations and steps we should address:

CSF files need to remain statically analyzable. The addition of helper functions like defineMeta could potentially obscure static analysis. It's important to ensure that any such utility does not interfere with static analysis required by Storybook's tools.

There seems to be a concern about the integration between defineMeta and a potential defineStory function. The issue #18228 refers to having a declarative way to define stories which defineMeta alone does not provide. How the two can be combined while retaining type inference needs to be discussed further.

Given the potential implications for CSF's design and static analysis capabilities, I recommend formalizing your proposal through the RFC (Request for Comments) process as your PR raises questions that warrant broader discussion with the core team and community. Writing an RFC in the GitHub Discussions will allow for collaborative exploration of how defineStory and defineMeta can and should be used together. It also provides a space for addressing any concerns related to static analyzability and integration with existing and future Storybook APIs.

For now, I will close this PR. Please proceed with the following steps:

  1. Open a GitHub Discussion with your RFC outlining your proposal for defineMeta and how it could potentially work with a defineStory utility.
  2. Engage with the feedback and collaborate with the community and the core team to refine the proposal.
  3. If your RFC gains consensus, the next steps would include implementing feedback from the discussion, ensuring integration aspects are covered, and opening a new PR for review by the Storybook maintainers.

@AriPerkkio
Copy link
Contributor Author

Thanks for the comments @valentinpalkovic.

This PR considers only defineMeta and not defineStory utility. It also keeps CSF's statically analyzable while allowing a bit different syntax for Meta and its default export.

I'll look into about writing up a RFC about defineMeta.

@kasperpeulen
Copy link
Contributor

kasperpeulen commented Dec 14, 2023

@AriPerkkio
Just for reference, some time ago I also proposed this:
#20849 (comment)

But it was rejected in favour of using satisfies instead.
I still belief factories would be the ideal way, and more and more people in the storybook team are getting on board with that.

I want to propose this style for CSF4. Ideally we would have something that makes sure the relationship between meta and story is understood out of the box, without much type hassling.

At this moment, we recommend:

import type { Meta, StoryObj } from '@storybook/react';

import { Button } from './Button';

const meta = {
  component: Button,
} satisfies Meta<typeof Button>;

export default meta;
type Story = StoryObj<typeof meta>;

export const Primary: Story = {
  args: {},
};

With your proposal, we can clean this up as:

import type { Meta, StoryObj } from '@storybook/react';

import { Button } from './Button';

const meta = defineMeta({
  component: Button,
});

export default meta;
type Story = StoryObj<typeof meta>;

export const Primary: Story = {
  args: {},
};

Which is an improvement, but if we are gonna recommend factories, I would like to go all the way:

import type { Meta, StoryObj } from '@storybook/react';

import { Button } from './Button';

const { meta, defineStory } = defineMeta({
  component: Button,
});

export default meta;

export const Primary = defineStory({
  args: {},
});

This looks a bit cleaner to me. And this would also allow us to get rid of type annotations for stories.

However, for CSF4, we might even go further. As one problem with our TS support is that we can not infer the parameters that are added by addons. One other idea for CSF4 is to make the relationship between addons/preview.ts and stories completely explicit (even on runtime).

Instead of main.ts and preview.ts files, I am thinking about renaming them as:

  • storybook.config.ts file that lives in the root of the project (just like a vite.config.ts file)
  • stories.config.ts file that lives in the root of your src directory (so that it can be easily imported)

The main difference is that storybook.config instructs the CLI, the builders and the manager.
And stories.config.ts instructs what is needed in the "canvas" (what can also be used outside of storybook).

In that line, we could have something like:

// stories.config.ts
import { defineConfig } from '@storybook/react';
import { mswLoader } from 'addon-msw';

export default defineConfig({
  loaders: [
    mswLoader,
  ],
  decorators: [...],
})
// Card.stories.ts
import config from '~/stories.config.ts'
import { Card } from './Card';
import { setSystemTime } from '@storybook/test'; 

// maybe just call it extend, as in some sense meta is just extending the config of preview
const { defineStory } = config.extend({ 
  component: Card,
  args: {
    disabled: false, // disabled type inferred from Card
  },
  argTypes: {
    // Another idea, is to make $-args, not end up in the Component, but can be used for other purposes
    $date: 'date',
  },
  // we are considering beforeRender for CSF4
  beforeRender: ({ args }) => {
    // TS knows about this $date, because it is inferred from argTypes
    if (args.$date) setSystemTime(args.$date)
  },
})

// we might not need to export meta explicitly in CSF4, so jeej

export const Default = defineStory({ 
  args: {
    $date: new Date(2020, 11, 24) // special christmas UI!
  },
  parameters: [
    msw: [
      http.get('/endpoint', () => Response.json({...})),
      // parameters.msw type inferred all the way up from mswLoader!
    ],
  ], 
})

Btw, these are all just my own thoughts and not yet accepted in any way.

@AriPerkkio
Copy link
Contributor Author

Those are great improvement ideas for Typescript support. I really like the proposed storybook.config.ts and stories.config.ts. 👍

I myself don't usually type stories at all with StoryObj<typeof meta>. I just export functions returning JSX and that's it (I guess that's CSF2 style?). My use case considers only making Meta typing easier.

@kasperpeulen do you think I should still open RFC for defineMeta? I'm not anymore sure if it would be accepted eventually.

@valentinpalkovic
Copy link
Contributor

I think ideas for CSF4 are definitely welcome to be put into a RFC. Maybe @kasperpeulen and you might collaborate to create one :)

@shilman
Copy link
Member

shilman commented Dec 19, 2023

@AriPerkkio Thanks so much for this PR. I think we're all in agreement that this is the way to go.

My primary concern is not about the whether these type-safe factory functions are a good idea or whether they are statically analyzable, but when and how to introduce them. Every time we change the story format it is hugely disruptive for our users, who have written literally millions of stories in the old format.

Even though most of the stories can be automatically transformed from CSF 1 to 2 and 2 to 3 with codemods, is still something new to learn for users. Also, blog posts and docs go out of date.

Therefore, for CSF4, I hope we can introduce a few different improvements at once, similar to some of the stuff @kasperpeulen has proposed above. In addition to everything said here, next year we'll be focusing on making it easy to build entire pages in Storybook. I suspect in the course of that, we'll pick up a few more CSF4 constructs.

If you and Kasper create an RFC for these factory functions, we can discuss there. I'm confident we can figure out a good way to release this experimentally in 8.x so that early adopters can use it while the entirety of CSF4 takes shape.

@AriPerkkio
Copy link
Contributor Author

I have now opened RFC considering defineMeta: #25319

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.

Add helper methods defineStory and defineMeta to enhance typescript support
4 participants