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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[RFC] feat: create defineArgs initial implementation #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions src/core/__tests__/parser.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { describe, expect, it } from 'vitest'
import { compileScript, parse as parseSFC } from 'vue/compiler-sfc'
import { extractDefineArgs, parse } from '../parser'

describe('extractDefineArgs', () => {
// Question: `defineArgs` is a fake "macro" we created. Could we actually
// implement it as a macro, so that it supports generics to allow
// for `defineArgs<typeof Button>() and then infer the type of the args
// with autocomplete for props and slots?
it('should extract args', () => {
const { descriptor } = parseSFC(`
<script setup>
defineArgs({foo: 'bar'})
</script>
<template>
<h1>Test</h1>
</template>
`)

const resolved = compileScript(descriptor, { id: 'test' })

expect(extractDefineArgs(resolved)).toMatchObject({
args: {
foo: 'bar',
},
})
})


})

describe('parse', () => {
it('should parse the args', () => {
const { args } = parse(`
<script setup>
defineArgs({foo: 'bar'})
</script>
<template>
<Stories>
<Story id="test" title="test" template="<h1>test</h1>"></Story>
</Stories>
</template>
`)
expect(args).toMatchObject({
args: {
foo: 'bar',
},
})
})

it("should expose the args to the template", () => {
const { args } = parse(`
<script setup>
const args = defineArgs({default: 'hello world'})
</script>
<template>
<Stories>
<Story id="test" title="test">
<b-button>{{ args.default }}</b-button>
</Story>
</Stories>
</template>
`)
expect(args.default).toBe('hello world')
})

it("should parse the args from the template and allow for local story overrides", () => {
const { stories } = parse(`
<script setup>
const args = defineArgs({default: 'hello world'})
</script>
<template>
<Stories>
<Story id="test" title="test" :args="{default: 'foo bar'}">
<button>{{ args.default }}</button>
Copy link
Contributor Author

@floroz floroz Sep 6, 2023 •
edited
Loading

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.

Copy link
Owner

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.

Copy link
Contributor Author

@floroz floroz Sep 8, 2023 •
edited
Loading

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}".

Copy link
Owner

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?

Copy link
Contributor Author

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 onto Stories provide type information to Story so that it can provide inference for the Story :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 the Story.

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 onto Story. How do we want to share type information coming from meta with the smallest footprint?

Copy link
Owner

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

Copy link
Contributor Author

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:

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

  1. 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:

  1. Volar plugin capturing the generic signature passed to defineMeta and providing that to each <Story />

  2. Adding the : component prop to each <Story /> to avoid polluting the functionality and semantics of existing args and argTypes.

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?

Copy link
Contributor Author

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)?

Copy link
Owner

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.

  1. I think we can easily support all three version: defineMeta for a uniform treatment, and the single defineArgTypes etc methods if someone prefers that (or wants to define a special arg type for a single story), and passing the options via a central Stories component (which would be closer to the Option API of vue). The implementation overhead is there, but not too much in my opinion.
  2. I still don't really get the advantage over component (or component-instance) over argTypes. 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.
  3. 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.

Copy link
Contributor Author

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.

</Story>
</Stories>
</template>
`)

// expect the args to be overwritten
// TODO: test that the template is compiled to <button>foo bar</button>
// right now it is <button>{{ args.default }}</button>
// is it correct? at which stage should `args` be compiled into their actual values?
expect(stories[0].template).toContain('foo bar')
})
})


74 changes: 71 additions & 3 deletions src/core/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { SFCDescriptor } from 'vue/compiler-sfc'
import type { SFCDescriptor, SFCScriptBlock } from 'vue/compiler-sfc'
import {
compileScript,
compileTemplate,
Expand All @@ -12,6 +12,8 @@ export interface ParsedMeta {
title?: string
component?: string
tags: string[]
// eslint-disable-next-line @typescript-eslint/no-explicit-any
args: Record<string, any>
}

export interface ParsedStory {
Expand All @@ -26,20 +28,25 @@ export function parse(code: string) {
if (descriptor.template === null) throw new Error('No template found in SFC')

const resolvedScript = resolveScript(descriptor)
const { args } = extractDefineArgs(resolvedScript)

// The `parseTemplate` should not build the meta, but just expose the raw data. The meta should be built by combining the resoluvedScript and the parsedTemplate.
const { meta, stories } = parseTemplate(descriptor.template.content)
const docsBlock = descriptor.customBlocks?.find(
(block) => block.type === 'docs'
)
const docs = docsBlock?.content.trim()
// TODO: Before exposing the stories, we should merge the args from the script and the template, where the ones coming from the template override the ones from the script.
return {
resolvedScript,
meta,
stories,
docs,
args,
}
}

function parseTemplate(content: string): {
export function parseTemplate(content: string): {
meta: ParsedMeta
stories: ParsedStory[]
} {
Expand All @@ -63,11 +70,15 @@ function parseTemplate(content: string): {
const root = roots[0]
if (root.type !== 1 || root.tag !== 'Stories')
throw new Error('Expected root to be a <Stories> element.')

const meta = {
title: extractTitle(root),
component: extractComponent(root),
tags: [],
}
// Here we should extract the args from the defineArgs call, within the script.
// But we are building this meta from `parseTemplate`. The meta should be built from `parse` instead.
// args: extractDefineArgs(resolvedScript),
} satisfies ParsedMeta

const stories: ParsedStory[] = []
for (const story of root.children ?? []) {
Expand Down Expand Up @@ -118,6 +129,63 @@ function extractPlay(node: ElementNode) {
? prop.exp?.content.replace('_ctx.', '')
: undefined
}
/**
* This function should extract the args declared on the individual stories.
*/
function extractArgs(node: ElementNode): Record<string, unknown> {
const prop = extractProp(node, 'args')
console.log('prop', prop)
return {}
}

/**
* Open questions:
*
* - How to handle multiple defineArgs calls?
* - How to handle defineArgs calls in the template?
* - What should be the merging mechanism for args, so that args declared in the template override args declared in the script?
*/
export function extractDefineArgs(resolvedScript: SFCScriptBlock | undefined) {
/* eslint-disable @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-call */
if (!resolvedScript || !resolvedScript?.scriptSetupAst) {
return { args: {} }
}

const ast = [
...(resolvedScript?.scriptAst ?? []),
...(resolvedScript?.scriptSetupAst ?? []),
]

const filtered = ast.filter(
// @ts-expect-error expression is not on the type
(node) => node.expression?.callee.name === 'defineArgs'
)

// 1. we need to look through all function expressions in the script
const defineArgsCalls = filtered
.filter(
(node) => node.type === 'ExpressionStatement' && 'expression' in node
)
// @ts-expect-error expression is not on the type
.filter((node) => node.expression.callee.name === 'defineArgs')
// @ts-expect-error expression is not on the type
.map((node) => node.expression)

const args = {}

defineArgsCalls.forEach((node) => {
const arg = node.arguments[0]
if (arg.type === 'ObjectExpression') {
arg.properties.forEach((prop: any) => {
// @ts-expect-error expression is not on the type
args[prop.key.name] = prop.value.value
})
}
})

/* eslint-enable @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-call */
return { args }
}

// Minimal version of https://github.com/vitejs/vite/blob/57916a476924541dd7136065ceee37ae033ca78c/packages/plugin-vue/src/main.ts#L297
function resolveScript(descriptor: SFCDescriptor) {
Expand Down