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

React: Sound arg types for CSF3 #19238

Merged
merged 1 commit into from Oct 11, 2022
Merged

React: Sound arg types for CSF3 #19238

merged 1 commit into from Oct 11, 2022

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Sep 23, 2022

Issue: #13747

Depends on this CSF pull request:
ComponentDriven/csf#49

What I did

Typesafe args

Changed StoryObj and Meta so that we have increased typesafety for when the user provides args partially in meta.

Considering a Component like this:

interface ButtonProps {
  label: string;
  disabled: boolean;
}

declare const Button: (props: ButtonProps) => JSX.Element;

It is valid to provide args like this:

// ✅ valid
const meta = {
  component: Button,
  args: { disabled: false },
} satisfies Meta<typeof Button>;

export default meta;

type Story = StoryObj<typeof meta>;

export const Basic = {
  args: { label: 'good' }
} satisfies Story;

While it is invalid too forget an arg, in either meta or the story:

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

export const Basic = {
  args: { label: 'good' }
} satisfies Story;

// ❌ invalid
const meta = {
  component: Button,
  args: { label: 'good' }
} satisfies Meta<typeof Button>;

export const Basic = {} satisfies Story;

Changed Meta to make sure both a Component, as the Props of the component can be used:

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

export default meta;

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

export default meta;

Typesafe decorators/loaders

Decorators now accept a new generic arg argument to be specified:

const withDecorator: DecoratorFn<{ decoratorArg: number }> = (Story, { args }) => (
  <>
    Decorator: {args.decoratorArg}
    <Story args={{ decoratorArg: 0 }} />
  </>
);

And the type of meta/story will check if this arg is part of the generic type:

type Props = ButtonProps & { decoratorArg: number };

const meta = {
  component: Button,
  args: { disabled: false },
  decorators: [withDecorator],
} satisfies Meta<Props>;

type Story = StoryObj<typeof meta>;
const Basic: Story = { args: { decoratorArg: 0, label: 'good' } };

How to test

You can test it by playing with test-d file.

@kasperpeulen kasperpeulen force-pushed the future/sound-arg-types branch 2 times, most recently from 26a8200 to 6f92476 Compare October 4, 2022 09:43
@kasperpeulen kasperpeulen changed the title WIP: Sound arg types Sound arg types Oct 6, 2022
@shilman shilman changed the title Sound arg types React: Sound arg types for CSF3 Oct 6, 2022
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.

Awesome! I think we need to make this as non-breaking as possible so that we can postpone figuring out the automigration strategy for now. Will review in more detail next time 🙏

code/renderers/react/src/public-types.ts Outdated Show resolved Hide resolved
code/renderers/react/src/public-types.ts Show resolved Hide resolved
code/renderers/react/src/types.ts Outdated Show resolved Hide resolved
code/renderers/react/src/__test-dts__/utils.ts Outdated Show resolved Hide resolved
code/renderers/react/src/__test-dts__/CSF3.test-d.tsx Outdated Show resolved Hide resolved
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

This looks really great. I have a suggestion for one more test.

code/renderers/react/src/public-types.ts Outdated Show resolved Hide resolved
code/renderers/react/src/__test-dts__/CSF3.test-d.tsx Outdated Show resolved Hide resolved
@@ -1,4 +1,5 @@
/* eslint-disable global-require */
import * as api from '.';
Copy link
Member

Choose a reason for hiding this comment

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

can you specify the path e.g. './index'

@kasperpeulen kasperpeulen merged commit cb2cd99 into next Oct 11, 2022
@kasperpeulen kasperpeulen deleted the future/sound-arg-types branch October 11, 2022 11:28
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.

None yet

5 participants