-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
[RFC] feat: create defineArgs
initial implementation
#73
base: main
Are you sure you want to change the base?
Conversation
defineArgs
initial implementationdefineArgs
initial implementation
<template> | ||
<Stories> | ||
<Story id="test" title="test" :args="{default: 'foo bar'}"> | ||
<button>{{ args.default }}</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tobiasdiez this is probably the first important point to discuss.
I do like that the experience for clients would feel extremely close to the native Storybook renderer, where you have the render(args) { return <Story /> }
where args
is an object available within the context of the Stories.
The difference here would be that our args
will be within a global scope of the file, but would assume different values based on the Story in which its used.
The vue approach of such a feature, would be a scoped slot otherwise:
<script setup>
defineArgs({default: 'hello world'})
</script>
<template>
<Story :args="{default: 'foo bar'}">
<template #story="{ args }">
<button>{{ args.default }}</button>
</template>
</Story>
</template>
What do you think? The first is closer to Storybook but would involved blackbox magic, the latter is maybe more verbose but closer to Vue's architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem I see with the global args definition, is that one naturally would expect something like
<script setup>
const argsOne = defineArgs({label: 'hello world'})
const argsTwo = defineArgs({label: 'hello world 2'})
</script>
<template>
<Stories>
<Story id="one" :args="{label: 'foo'}">
<button>{{ argsOne.label }}</button> # the default should be equivalent to <button>foo</button>
</Story>
<Story id="two" :args="{label: 'bar'}">
<button>{{ argsTwo.label }}</button> # the default should be equivalent to <button>bar</button>
</Story>
</Stories>
<template>
to work. But this seems hard to implement - how is the story knowing which of the argsOne/Two
it is supposed to overwrite?
One the other hand, I really like the scoped slots version which by definition provides local args
for each story. I would only rename the args
prop of the story in your example to initial-args
(or default?) to make the purpose a bit clearer.
Another small point: Maybe we can pass the args directly as the slot argument, so that you can conveniently destruct it as in the following example:
<Story v-slot="{ label }">
<button>{{ label }}</button>
</Story>
According to https://github.com/storybookjs/storybook/blob/d8c5bbae3f40b2f1293c210e220522a6fbbf87bd/code/renderers/vue3/src/public-types.ts#L48C17-L48C17 and https://github.com/ComponentDriven/csf/blob/4c735fea4f0c9605b93497238303cb4ab9304727/src/story.ts#L170-L173, a story render function can only accept args
and context
, so what we would loose with the above is the context
variable. Maybe that can be supported by prefixing it with a dollar sign?
<Story v-slot="{ label, $context }">
<button>{{ label }}: {{ $context.title }}</button>
</Story>
But its also not a big pain to have v-slot
emit { args, context }
as the default storybook renderer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Args architecture
I do agree on the scoped slot preference. It's also much closer to the Vue's native experience and removes a layer of "magic" from final users.
I wouldn't destructure the args
object since it allows to keep the same render function signature of args, context
, but also allows to quickly understand in the template which variables are coming from the args
, and which from the global script context.
<Story v-slot="{ args }">
<Dialog v-model="args.modelValue" :label="label"/>
</Story>
TypeScript support
What about TypeScript support? Would there be a way for clients to declare their component's type and have args
IDE autocompletion for props and slots?
Where should that declaration occur? in the defineArgs
? I think it makes sense to keep defineArgs
instead of initial-args
prop since it removes the need to also come up with a different convention for the pro naming on the story.
Design proposal
defineArgs
as call site to provide type inference
defineArgs
sets the default args
as the Meta
in Storybook. We will follow the same approach for argTypes
(defineArgTypes
) and parameter
(defineParamaters
).
defineArgs
support for TypeScript would be an important feature to keep the experience in using the addon on
the same level as the Story CSF.
Local override using :args
prop on the <Story />
synthetic template
We use :args
on the local Story component to provide the local override
Expose args
to the template via scoped slot on <Story />
synthetic template
Should it be the default
slot? should we used a name slot like #render
since its meant to replicate the render
function in CSF?
Sketch of final usage example from clients
<script setup lang="ts">
const openModal1 = ref(false);
const openModal2 = ref(true);
defineArgs<typeof Dialog>({ label: 'Default Label'});
</script>
<template>
<Story v-slot="{ args }">
<Dialog v-model="openModal1" :label="args.label"/>
</Story>
<Story v-slot="{ args }" :args="{ label: 'Custom Label'}">
<Dialog v-model="openModal2" :label="labels"/>
</Story>
</template>
Next steps
If happy with the design, the next step should be to create a PR to have a working implementation (without TypeScript support) of defineArgs
, :args
property and v-slot="{args}"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, I agree with all your points. Only question I still have is how the defineArgs
would provide the types. I don't think this is possible with the current implementation of volar.
Instead, I would introduce a macro defineArgTypes
and pass the result of that call to each story (ideally it would suffices to pass it once to the Stories
component, but I don't think type interference for child components is implemented in volar, see #74). So the result would be something like:
<script setup lang="ts">
const openModal1 = ref(false);
const openModal2 = ref(true);
const argTypes = defineArgTypes<typeof Dialog>({ label: { control: 'text' } });
// argTypes has two purposes: overwrite the controls and also carry the type information
</script>
<template>
<Stories :component="Dialog" :args="{ label: 'Default label' }">
<Story v-slot="{ args }" :argTypes="argTypes">
<Dialog v-model="openModal1" :label="args.label" :description="args.desc" />
</Story>
<Story v-slot="{ args }" :args="{ label: 'Custom Label'}">
<Dialog v-model="openModal2" :label="labels"/>
</Story>
<Stories>
</template>
In addition we could provide a defineArgs
macro that provides a convenient way to define args in the script
block without loosing intellisense etc, but I would leave it for a follow-up PR.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking the macro to the Stories
to spread the type inference is definitely a great idea.
The issue I see is that if we want to leverage defineArgTypes
as the function to take the generic, we are forcing clients always to define argTypes
even when not required.
At this point, since the type inference is essential for clients and we need to provide good ergonomics to declare types/consume them on Stories
, I would suggest following the Storybook CSF's structure and creating a defineMeta
macro.
Whenever a Story CSF is created, all clients must create a meta: Meta<typeof Component>
object, whether they need to explicitly declare args
, argTypes
, etc., or not.
<script setup lang="ts">
const meta = defineMeta<typeof Dialog>({
argTypes: { label: { control: 'text' } },
args: { label: 'Custom Label' }
});
</script>
<template>
<Stories :component="Dialog" :meta="meta">
<Story v-slot="{ args }">
<Dialog v-model="openModal1" v-bind="args" />
</Story>
<Story v-slot="{ args }" :args="{ label: 'Custom Label'}">
<Dialog v-model="openModal2" v-bind="args"/>
</Story>
<Stories>
</template>
The two challenges here are:
-
Can the
meta
type passed ontoStories
provide type information toStory
so that it can provide inference for theStory
:args
prop? -
If not, then we would need to connect the
meta
on<Story :meta="meta" />
for each Story to provide the type inference. That could lead to some confusion due to different concepts of Meta in CSF. I wonder if, at that point, we could use the new Vue generics syntax on theStory
.
Looking at this usage
<Story v-slot="{ args }" :args="{ label: 'Custom Label'}">
<Dialog v-model="openModal2" v-bind="args"/>
</Story>
I also wonder, do we need to implement the complexity of local :args
prop overriding the one coming from the meta
?
A client who wants a local override, since they always have to work with the template, compared to the StoryObj
of CSF, could simply do:
<Story v-slot="{ args }" >
<Dialog v-model="openModal2" v-bind="args" label="Custom Label"/>
</Story>
That would remove some complexity from our side and keep the footprint of the addon smaller on their files?
To recap
<script setup lang="ts">
const meta = defineMeta<typeof Dialog>({
argTypes: { label: { control: 'text' } },
args: { label: 'Custom Label' }
});
</script>
<template>
<Stories :component="Dialog" :meta="meta">
<Story v-slot="{ args }">
<Dialog v-model="openModal1" v-bind="args" />
</Story>
<Story v-slot="{ args }" >
<Dialog v-model="openModal2" v-bind="args" label="Custom Label"/>
</Story>
<Stories>
</template>
Pending to define:
- type inference from
Stories
might not be passed ontoStory
. How do we want to share type information coming frommeta
with the smallest footprint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really what would we need
<Stories />
for?
It's there mostly to make eslint happy/make it conform to vue requirements. Vue 2 template has to have exactly one root component. This is no longer the case for vue 3, so yes, we could use a useMeta
macro as an alternative to specify global information.
It's pretty clear we need to pass an individual prop to each
<Story />
to get the correct type inference from the generic type, but I think that using an existing Storybook functionality might be confusing for the end user.But, if we were forced to pass a prop to declare the component's types, which would be the most intuitive one? Well, we already have it, it's
:component
on<Stories />
.
The problem is that, for standard storybooks, one can manually overwrite argTypes
for a given story, and add additional args that are not coming from the component. So this needs to be supported as well. But for component this is not the case (at least I couldn't find it in the docs).
What I just realized is that one can write plugins for volar (see eg https://github.com/vue-macros/vue-macros/tree/main/packages/volar or https://github.com/Tahul/pinceau/blob/main/src/volar.ts). With this one could provide the type information from general define...
macros and push down the type information from a central Stories
element (e.g. just replicate argTypes/component
props from the parent to the Story
childs if they don't overwrite it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that, for standard storybooks, one can manually overwrite argTypes for a given story, and add additional args that are not coming from the component. So this needs to be supported as well. But for components, this is not the case (at least I couldn't find it in the docs).
Yes, for args
, argTypes
, parameters
, etc. they all will need a global macro to set up the default for the meta
, and a local prop on <Story />
for the local override.
I think that's quite clear and there's no ambiguity.
Let's describe the problems we need to solve:
1. Users need to set up the default of configuration on their Meta
, within the script
block.
Here we've got two approaches to choose from:
- Create
defineMeta<typeof Component>
as a way to access the entire schema of the Storybook Meta.
This would be my preferred approach, a unified interface exposing 1:1 the Meta<typeof Component
.
For the first iteration, we could exclusively support args
to validate the concept and review the implementation.
- Create fragments of the meta accessible by dedicated macros (
defineArgs
,defineArgTypes
,defineParameters
).
The issue I would have with this approach is that each "fragment" to access the Meta will require type information in the generic signature
<script setup lang="ts">
defineArgs<typeof Button>({});
defineArgTypes<typeof Button>({});
</script>
While the defineMeta
would contain all the options within a single call, hence reducing the type footprint in the file.
<script setup lang="ts">
defineMeta<typeof Button>({
args: {},
argTypes: {}
});
</script>
2. Users need to access type information on their <Story />
from the props exposed by each slot.
There are two approaches to solving this problem:
-
Volar plugin capturing the generic signature passed to
defineMeta
and providing that to each<Story />
-
Adding the
: component
prop to each<Story />
to avoid polluting the functionality and semantics of existingargs
andargTypes
.
Once we have a supported solution from Volar, this property can be deprecated gradually to avoid breaking change (we can just ignore that prop internally once we make the switch of the addon implementation).
:component
is still not ideal, so for example since in defineMeta
we would have { title: 'components/Button', component: Button }
, we can also consider just creating a descriptive and dedicated prop for this specific issue, to avoid any potential confusion with Storybook functionalities.
Something like :component-instance
for example.
3. Change the addon approach from string token, to export Vue Components and functions
This is something I'd like you to confirm to make sure we're on the same page in terms of potential implementation.
With these new features, unless we create a Volar plugin, we would need to create real components that are exported to the users.
It will probably need to be a dedicated export point to avoid conflict with the output of the Storybook addon
import { Story, defineMeta } from 'storybook-vue-addon/setup';
// I am a function with a `.d.ts` file
defineMeta<typeof Button>();
Is my understanding correct here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the above, we could start tackling some work in parallel.
If my understanding of point 3. is correct, I could tackle point 1 (create the macro and the Story component with scoped slots, and local overrides) while you research further into point 2 (Volar plugin to avoid creating the type prop on each Story)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should start extracting some of these points to new issues, since some of them are actually not strictly speaking involved args support.
- I think we can easily support all three version:
defineMeta
for a uniform treatment, and the singledefineArgTypes
etc methods if someone prefers that (or wants to define a special arg type for a single story), and passing the options via a centralStories
component (which would be closer to the Option API of vue). The implementation overhead is there, but not too much in my opinion. - I still don't really get the advantage over
component
(orcomponent-instance
) overargTypes
. The latter is supposed to define what args are available (and how they are rendered), so a natural place to provide typing information for the args. Especially since you can overwrite/append to the automatically derived component-based args. - For me this would be only for the type support. So we don't really need to provide a full-fledge implementation for these commands, they only need to expose the correct types. At the very beginning of the development of the plugin, I considered really using
vue
to do the compilation to csf (instead of the manual parsing using the vue ast) but couldn't really find a way to do this. In principle, I would be open for such a shift. (But again, this is not strictly needed to support args).
But yes, I will try to play around with the volar plugin and see whats possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea @tobiasdiez, I will open a few new issue to move forward these discussions within the right context.
Regarding 1) and 2):
I am okay with using argTypes
as the call site to provide the type inference. My concern was only around the fact that end users will need to declare them even when they want to use whatever default is automatically inferred.
defineArgTypes<typeofButton>() // no arg types are actually declared.
But honestly, after discussing this at length, I think all alternatives have equal trade-offs, so we can pick this one until we can provide some type injection to <Story />
from the <Stories :component />
directly.
Regarding not exposing any implementation, but only types within the global scope for those macros, does this mean we want to expose a types.d.ts
for user to import in their tsconfig.json
?
Would the usage look like this?
"compilerOptions": {
"types": ["storybook-vue-addon"]
}
I am going to send you an email regarding some admin questions. For the next steps, I will open some new issues to track the discussion we've been having here.
Summary
This is an initial sketch to discuss a potential approach, API design and evaluate trade-offs.
I will update the summary as necessary as we review the first feedback