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

[Bug] [next] Type helper Args for setTemplate snippet doesn't satisfy component props interface #216

Open
xeho91 opened this issue Oct 18, 2024 · 5 comments
Assignees
Labels
bug Something isn't working has workaround This issue has a possible workaround help wanted Extra attention is needed next Related to the next version

Comments

@xeho91
Copy link
Collaborator

xeho91 commented Oct 18, 2024

Note

This case has a workaround.

Actually, while optional children work in that canary version, other props being required causes an error now.

Minimal example:

Button.stories.svelte:

<script context="module" lang="ts">
  import { defineMeta, setTemplate, type Args } from "@storybook/addon-svelte-csf";

  import Button from "./Button.svelte";

  const { Story } = defineMeta({
    component: Button,
    tags: ["autodocs"],
    args: {
      children: "Click me",
      variant: "filled",
    },
    argTypes: {
      variant: { control: "select", options: ["filled", "outlined"] },
      children: { control: "text" },
    },
  });
</script>

<script lang="ts">
  setTemplate(template);
</script>

{#snippet template({ children, ...args }: Args<typeof Story>)}
  <Button {...args}>{children}</Button>
{/snippet}

<Story name="Primary" args={{ variant: "filled" }} />

Button.svelte:

<script lang="ts">
  import type { Snippet } from "svelte";

  interface Props {
    variant: "filled" | "outlined";
    children?: Snippet;
  }

  const { variant, children }: Props = $props();
</script>

<button type="button" class={variant}>
  {@render children?.()}
</button>

Here's the error:

Image

Originally posted by @tomasz13nocon in #214

I need help on deciding how the type Args should reflect input from defineMeta.

Currently, there's a mismatch in the above example.
Component prop variant is not optional.
Once you hover on the args inside the {#snippet template({ children, ...args }: Args<typeof Story>)}, LSP type definition will show:

{
    variant?: "filled" | "outlined" | undefined;
}
@xeho91 xeho91 self-assigned this Oct 18, 2024
@JReinhold
Copy link
Collaborator

After giving it some thought I think we should just remove the MapSnippetsToAcceptPrimitives type completely.

Its intent is to allow a snippet prop like mySnippet: Snippet to be defined as args: { mySnippet: 'some primitive' } because that's a lot easier to work with, especially when using Controls. But currently that requires users to define templates that converts the primitive arg to a snippet, like what we do in our Button example with the children snippet:

{#snippet template({ children, ...args }: Args<typeof Story>, context: StoryContext<typeof Story>)}
<Button {...args}>{children}</Button>
{/snippet}

So our types say "feel free to use a primitive instead of a snippet", but in reality we require users to do this conversion explicitly. As a user, with this type, I'd expect the addon to do this conversion internally somehow. But it doesn't, and I don't think it should (or even can).

So short term I think we should remove MapSnippetsToAcceptPrimitives. That will cause types to be "wrong" when users set a snippet arg as a primitive, and so longer term we want that case to be easier, probably by making it easier to modify the type of Args as we've attempted before with no luck.

@xeho91
Copy link
Collaborator Author

xeho91 commented Oct 25, 2024

After giving it some thought I think we should just remove the MapSnippetsToAcceptPrimitives type completely.

Its intent is to allow a snippet prop like mySnippet: Snippet to be defined as args: { mySnippet: 'some primitive' } because that's a lot easier to work with, especially when using Controls. But currently that requires users to define templates that converts the primitive arg to a snippet, like what we do in our Button example with the children snippet:

addon-svelte-csf/examples/Button.stories.svelte

Lines 40 to 42 in b74aee0

{#snippet template({ children, ...args }: Args, context: StoryContext)}
<Button {...args}>{children}
{/snippet}
So our types say "feel free to use a primitive instead of a snippet", but in reality we require users to do this conversion explicitly. As a user, with this type, I'd expect the addon to do this conversion internally somehow. But it doesn't, and I don't think it should (or even can).

So short term I think we should remove MapSnippetsToAcceptPrimitives. That will cause types to be "wrong" when users set a snippet arg as a primitive, and so longer term we want that case to be easier, probably by making it easier to modify the type of Args as we've attempted before with no luck.

I agree. This type helper MapSnippetsToAcceptPrimitives brought more problems in attempting to enhance the typing experience.

I wonder if we could instead provide some small utility function e.g. children() which will use new createRawSnippet to speed up writing stories.
This is what I have in my mind:

const { Story } = defineMeta({
  args: {
-    children: "Click me",   
+    children: children("Click me"),
  }
});

It's probably not the best idea to name this function as children(). Naming snippet() instead could potentially confuse with {#snippet} block from code readability perspective.


Regardless, I think this will not solve the original issue.

If we have a component which has a required prop - e.g. variant...
And we provide it via meta.args (using defineMeta())...
then in the current state, type helper Args will still mark args.variant to be optional.
And this will not make TypeScript happy unless you define variant manually, again, inside {#snippet} block. Below is the example how to shush the error using the example above:

{#snippet template({ children, ...args }: Args<typeof Story>)}
  <Button variant="filled" {...args}>{children}</Button>
{/snippet}

Since TypeScript only recognises static values, it might be possible for him to recognise (probably using const generic type parameters), but at the moment I didn't manage to get closer to achieving it.

xeho91 added a commit that referenced this issue Oct 25, 2024
This is outcome of discussion at:
#216
@JReinhold
Copy link
Collaborator

I wonder if we could instead provide some small utility function e.g. children() which will use new createRawSnippet to speed up writing stories. This is what I have in my mind:

const { Story } = defineMeta({
args: {

  • children: "Click me",
  • children: children("Click me"),
    }
    });
    It's probably not the best idea to name this function as children(). Naming snippet() instead could potentially confuse with {#snippet} block from code readability perspective.

I like the direction here, but I think we want it to be the other way around. The point is that we want the actual args to be primitives like strings or numbers, so they are easily modifiable in Controls. If the args are snippets, they'll become functions in controls which are not easy to work with. So this helper function should be applied after the arg, so the arg is primitive, and then it becomes a snippet when passed to the component.

I'm not entirely sure how we want to achieve that yet, or if it's simply just easier for users to do that when defining their story templates.

If we have a component which has a required prop - e.g. variant... And we provide it via meta.args (using defineMeta())... then in the current state, type helper Args will still mark args.variant to be optional.

Ah yeah that's a problem.

@xeho91 xeho91 added bug Something isn't working help wanted Extra attention is needed labels Nov 5, 2024
@xeho91
Copy link
Collaborator Author

xeho91 commented Nov 5, 2024

This case still occurs.

Given that the first generic type parameter for defineMeta<TCmp> which is Component<any> from svelte... would it be possible to infer meta.args from the provided meta input (first argument)?

At the moment, my experience with TypeScript is limited here to conclude if this is feasible or not. There has to be some sort of internal type helper operation that converts required component props to optional - if they were provided in the first argument - meta.args.


Workaround

If you have a component with required props and you're defining a shared template for setTemplate()... then you have to explicitly provide the required prop inside the snippet block for the template. Regardless if it was provided in the defineMeta first argument.

Important

And also before the spread expression tag with args, so it can be overridden in Story components.

Example:

 {#snippet template({ children, ...args }: Args<typeof Story>)} 
   <!--    👇 explicitly provided required prop  -->
   <Button variant="filled" {...args}>{children}</Button> 
   <!--                     👆 add spread expression tag at the end to allow overriding in Story components  -->
 {/snippet} 

@fivaz3
Copy link

fivaz3 commented Nov 17, 2024

being honest, this workaround is so troublesome, specially if you have to do this everywhere and with many props.

a better suggestion would be to do this instead:

<script module lang="ts">
        import type { ComponentProps } from 'svelte';
//         👆import ComponentProps from svelte 
</script>

<script lang="ts">
	setTemplate(template as any); // this typing here is useless anyway
</script>

then in the template declaration we do like this:

<!-- use it here with the Component as its generic👇-->
{#snippet template({ children, ...args }: ComponentProps<typeof Button>)}
	<Button {...args}>{children}</Button>
{/snippet}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has workaround This issue has a possible workaround help wanted Extra attention is needed next Related to the next version
Projects
None yet
Development

No branches or pull requests

3 participants