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

Add unmappedArgs to story context #68

Closed
wants to merge 1 commit into from

Conversation

tmeasday
Copy link
Contributor

NOTE: this changes the type of context.args to reflect that by this point args have been mapped and actually we no longer know what type they have in detail.

I'm not sure if this is a breaking change or if it is something that will mess people up @kasperpeulen?

It is strictly more correct because:

  1. Arg mapping can change the type of an arg
  2. Conditional controls can drop an arg
  3. Decorators can add/change args at any stage of the decorator pipeline.

I'd be happy to change it back to args: TArgs if we are worried about that. Keeping in mind that that type is not actually correct :).

For storybookjs/storybook#22135

@tmeasday
Copy link
Contributor Author

tmeasday commented Apr 21, 2023

@kasperpeulen maybe I have this backwards and the type of args that finally makes its way into the render function should be TArgs and the args that go over the channel (unmappedArgs) is the one we don't know the type of?

It's sort of a problem though, because (as decorators can change args), the type of context.args can change between each decorator + the final render function.

* The args that are passed on the context are mapped by arg mapping
* and can be filtered by conditional arg filters.
*/
args: Args;
Copy link
Contributor

Choose a reason for hiding this comment

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

So we kind of want to override the args from TArgs to Args here right?
Because they are defined as TArgs in StoryContextUpdate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess they should be Args in StoryContextUpdate too, after all.

@kasperpeulen
Copy link
Contributor

I'm not sure if this is a breaking change or if it is something that will mess people up @kasperpeulen?

Will unmappedArgs be the one that the user defined in the story? Or the one that merges the args of preview/meta and the story?

I think it is not breaking, because we weaken the type in some sense, as it becomes an object with values typed as any.
But people will loose autocompletion/type safety when using args in decorators, play function, loaders.
We can also type it as StrictArgs, which has unknown values, but then the user have to do runtime type checks, before they can use the values of the args in those places.

I kind of feel like keeping it TArgs, and accepting some incorrectness here indeed, and keep investigating how we can make this more sound. For example, we already do some type mapping, between the user provided args and the context args over here, so that the user doesn't have to provide action args:

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

type ActionArgs<TArgs> = {
  // This can be read as: filter TArgs on functions where we can assign a void function to that function.
  // The docs addon argsEnhancers can only safely provide a default value for void functions.
  // Other kind of required functions should be provided by the user.
  [P in keyof TArgs as TArgs[P] extends (...args: any[]) => any
    ? ((...args: any[]) => void) extends TArgs[P]
      ? P
      : never
    : never]: TArgs[P];
};

We could maybe go further and make sure that context action args are Jest.Mock as that is one arg mapping we do right? Are action everywhere a Jest.Mock type or only in the play function?

But yes, we can not take into account user arg mappings and decorator modifications.
I kind of envisioned that if you change the value in a decorator, the user might not to silence the compiler in some places:

const Component = (props: { label: string; setInDecorator: string }) => <></>;

const withDecorator: Decorator = (Story, { args }) => (
  <Story args={{ ...args, setInDecorator: 'adsf' }} />
);

const meta = { component: Component } satisfies Meta<Props>;

const Basic: StoryObj<typeof meta> = {
  args: { label: 'label', setInDecorator: null! /* trust me, we will set in decorator */ },
  decorators: [withDecorator],
};

@tmeasday
Copy link
Contributor Author

tmeasday commented Apr 23, 2023

Let's have a chat about it. I think we should just get our story (ha!) straight--what are TArgs? Are the they inputs of the component (inferrable in many frameworks), or the (composed) set of "unmapped" (+ undecorated) args that are defined in CSF and go over the channel?

Either way is there a principled way the user can tell us what the relation is between the two? Maybe something like:

type Mapper<T> = T & { setInDecorator: boolean };
const meta = Meta<Component, Mapper>;

@tmeasday
Copy link
Contributor Author

Closing this for now, will revisit in storybookjs/storybook#22228

@tmeasday tmeasday closed this Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants