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

ArgsFromMeta utility and generic ArgsStoryFn RT #51

Merged
merged 3 commits into from
Oct 22, 2022

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Oct 18, 2022

What I did:

ArgsFromMeta:

Added ArgsFromMeta type, this a type that can be used to infer the args from Meta that are unrelated to the component. It is used here:

https://github.com/storybookjs/storybook/blob/b42d9456cce20d06097d83f080f52ca65a9ba7b2/code/renderers/react/src/public-types.ts#L38-L59

export type StoryObj<MetaOrCmpOrArgs = Args> = MetaOrCmpOrArgs extends {
  render?: ArgsStoryFn<ReactFramework, any>;
  component?: infer Component;
  args?: infer DefaultArgs;
}
  ? Simplify<
      (Component extends ComponentType<any> ? ComponentProps<Component> : unknown) &
        ArgsFromMeta<ReactFramework, MetaOrCmpOrArgs>
    > extends infer TArgs
    ? StoryAnnotations<
        ReactFramework,
        TArgs,
        SetOptional<TArgs, Extract<keyof TArgs, keyof (DefaultArgs & ActionArgs<TArgs>)>>
      >
    : never
  : MetaOrCmpOrArgs extends ComponentType<any>
  ? StoryAnnotations<
      ReactFramework,
      ComponentProps<MetaOrCmpOrArgs>,
      ComponentProps<MetaOrCmpOrArgs>
    >
  : StoryAnnotations<ReactFramework, MetaOrCmpOrArgs>;

Basically, when StoryObj accepts a meta (StoryObj<typeof meta>), we need to find out the args that are passed to render/loader/decorators, which will be used as the actual args for StoryObj. This is common across the implementation of StoryObj across frameworks, so therefore I put it here.

The inference of the args of the component is framework specific, so has to stay in the renderer code.

Upgraded to tsup
Also upgraded many other dependencies, and added some github workflows.

📦 Published PR as canary version: 0.0.2--canary.51.084d9eb.0

✨ Test out this PR locally via:

npm install @storybook/[email protected]
# or 
yarn add @storybook/[email protected]

@kasperpeulen kasperpeulen requested a review from tmeasday October 18, 2022 08:48
@kasperpeulen kasperpeulen force-pushed the kasper/csf-3-improvements branch from ceabbe7 to 23af5af Compare October 18, 2022 15:09
@kasperpeulen kasperpeulen mentioned this pull request Oct 18, 2022
"@babel/cli": "^7.8.4",
"@babel/core": "^7.9.0",
"@babel/preset-env": "^7.9.5",
"@babel/preset-typescript": "^7.9.0",
Copy link

Choose a reason for hiding this comment

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

😍

tsconfig.json Outdated
"lib": ["es2015", "es2017", "dom"],
"module": "commonjs",
"moduleResolution": "Node",
"module": "ES2022",
Copy link

Choose a reason for hiding this comment

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

The storybook monorepo is still using:

 "target": "ES2020",
 "module": "CommonJS",

This seems like quite a jump. Is this going to impact our chrome 100+ support target?

Copy link
Member

Choose a reason for hiding this comment

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

In the storybook monorepo the tsconfig isn't used to determine the compile target at all

Copy link

Choose a reason for hiding this comment

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

Is it being used in this case? If so, is this the correct target?

Copy link
Member

Choose a reason for hiding this comment

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

It might not matter at all, depending on the source code. but let's play it safe and target 2020?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tomorrow, was a long day 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@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.

Looks good, although I'm not quite sure what it is for?

Comment on lines +338 to +342
export type ArgsFromMeta<TFramework extends AnyFramework, Meta> = Meta extends {
render?: ArgsStoryFn<TFramework, infer RArgs>;
loaders?: (infer Loaders)[];
decorators?: (infer Decorators)[];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a TS "test" for this? At the very least it would document what it is for :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, yeah, good point, I have tests for it in this PR:
https://github.com/storybookjs/storybook/pull/19512/files
But maybe good to put a simple test here as well.

I was not really sure where to put this. I could also have put it somewhere in the monorepo. Because StoryObj will now use Meta(OrArgs) as generic (typeof meta), I will need something like this in the public types of every renderer (we use it now only for react and svelte). But because there is also framework-specific inference (to infer the args of the component or specific stuff for action arg inference) I can not put all of StoryObj in this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test now

@yannbf
Copy link

yannbf commented Oct 19, 2022

Hey @kasperpeulen this looks good but your PR has no description, would be nice to explain what's the purpose with some small example use cases. This might help anyone that wants to revisit this PR in the future

@kasperpeulen
Copy link
Contributor Author

Hey @kasperpeulen this looks good but your PR has no description, would be nice to explain what's the purpose with some small example use cases. This might help anyone that wants to revisit this PR in the future

@yannbf Added a description and a test to make it a bit more clear hopefully 😅

@shilman
Copy link
Contributor

shilman commented Apr 3, 2023

🚀 PR was released in v0.1.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants