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

Vue3: Fix Conditional decorators #23029

Closed
wants to merge 33 commits into from
Closed
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
48f343d
revert to chaks vue3 impl
chakAs3 Jun 9, 2023
7d6626d
revert back test stories ( no need to spread )
chakAs3 Jun 9, 2023
c950f97
use shallow reactive to avoid deep reactivity
chakAs3 Jun 9, 2023
0bb0788
updatedArgs reinitialisation in first reducer
chakAs3 Jun 9, 2023
29efc3d
fix type checking
chakAs3 Jun 9, 2023
a5dc5b1
Merge branch 'next' into chaks/vue3-impl
chakAs3 Jun 9, 2023
2d2f7ea
fix playwright test runing issue
chakAs3 Jun 9, 2023
ba1da92
cleanup after test
chakAs3 Jun 9, 2023
1fb6d73
Merge branch 'next' into chaks/vue3-impl
chakAs3 Jun 9, 2023
4bc5b4d
simplify and improve visually the tests,
chakAs3 Jun 9, 2023
c939afb
refactory decorateStory for clarity and fix bugs
chakAs3 Jun 10, 2023
15f94d0
better refactory
chakAs3 Jun 10, 2023
5a7ce9f
disable fall through attribute
chakAs3 Jun 10, 2023
1ca0097
Merge branch 'next' into chaks/vue3-fix-cond-decorators
chakAs3 Jun 11, 2023
3c4a4fa
cleanup deps remove unessary and add necessary
chakAs3 Jun 12, 2023
f7ec799
refactory decorateStory with necessary fix
chakAs3 Jun 12, 2023
e5a69e3
remove forgotten console.log from other PR
chakAs3 Jun 12, 2023
0781211
remove unnecessary code
chakAs3 Jun 12, 2023
9a487b0
cleanup comments
chakAs3 Jun 12, 2023
eeea195
Merge branch 'next' into chaks/vue3-fix-cond-decorators
chakAs3 Jun 12, 2023
2825671
remove unnecessary code
chakAs3 Jun 12, 2023
cb44c68
cleanup and refactory
chakAs3 Jun 12, 2023
5503a11
rollback out of scope changes
chakAs3 Jun 12, 2023
b1e7a5e
fix type check
chakAs3 Jun 12, 2023
93b0c9d
revert test
chakAs3 Jun 12, 2023
94443e3
Merge branch 'next' into chaks/fix-vue3-cond-null-decorators
chakAs3 Jun 12, 2023
a062528
lockfile generation
chakAs3 Jun 12, 2023
895b01b
Merge branch 'next' into chaks/fix-vue3-cond-null-decorators
chakAs3 Jun 13, 2023
d26c25f
Merge branch 'next' into chaks/fix-vue3-cond-null-decorators
chakAs3 Jun 14, 2023
51bd30f
deps changes
chakAs3 Jun 15, 2023
3bcada9
fix type checking
chakAs3 Jun 15, 2023
aba16cb
add back reactivity to slots
chakAs3 Jun 15, 2023
2223cd1
cleanup fix types
chakAs3 Jun 15, 2023
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
1 change: 1 addition & 0 deletions code/frameworks/vue3-vite/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
"@storybook/vue3": "7.1.0-alpha.30",
"@vitejs/plugin-vue": "^4.0.0",
"magic-string": "^0.30.0",
"react": "^18.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

Could you please elaborate why this is needed?

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'll take it back it was a try

"vue-docgen-api": "^4.40.0"
},
"devDependencies": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export default {
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => {
return storyFn({
args: { object: { ...context.args } },
args: { object: context.args },
});
},
],
Expand Down
2 changes: 1 addition & 1 deletion code/lib/preview-api/template/stories/argTypes.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export default {
// Compose all the argTypes into `object`, so the pre component only needs a single prop
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) =>
storyFn({ args: { object: { ...context.argTypes } } }),
storyFn({ args: { object: context.argTypes } }),
],
argTypes: {
componentArg: { type: 'string' },
Expand Down
3 changes: 1 addition & 2 deletions code/lib/preview-api/template/stories/args.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ export default {
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => {
const { argNames } = context.parameters;
const args = { ...context.args };
const object = argNames ? pick(args, argNames) : args;
const object = argNames ? pick(context.args, argNames) : context.args;
return storyFn({ args: { object } });
},
],
Expand Down
8 changes: 5 additions & 3 deletions code/lib/preview-api/template/stories/decorators.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,15 @@ export const Hooks = {
// decorator that uses hooks
(storyFn: PartialStoryFn, context: StoryContext) => {
useEffect(() => {});

return storyFn({ args: { ...context.args, text: `story ${context.args['text']}` } });
},
// conditional decorator, runs before the above
(storyFn: PartialStoryFn, context: StoryContext) =>
context.args.condition
(storyFn: PartialStoryFn, context: StoryContext) => {
return context.args.condition
? storyFn()
: (context.originalStoryFn as ArgsStoryFn)(context.args, context),
: (context.originalStoryFn as ArgsStoryFn)(context.unmappedArgs, context);
},
],
args: {
text: 'text',
Expand Down
2 changes: 2 additions & 0 deletions code/lib/preview-api/template/stories/hooks.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ export default {
};

export const UseState = {
// parameters: { inheritAttrs: true },
args: { label: 'Clicked 0 times' },
decorators: [
(story: PartialStoryFn) => {
const [count, setCount] = useState(0);
Expand Down
1 change: 1 addition & 0 deletions code/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@
"node-gyp": "^9.3.1",
"nx": "16.2.1",
"nx-cloud": "16.0.5",
"playwright-core": "^1.35.0",
"prettier": "2.8.0",
"process": "^0.11.10",
"raf": "^3.4.1",
Expand Down
2 changes: 1 addition & 1 deletion code/renderers/vue3/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@
"@types/prettier": "2.7.2",
"@vue/vue3-jest": "29",
"typescript": "~4.9.3",
"vue": "^3.2.47",
"vue-tsc": "latest"
},
"peerDependencies": {
"lodash": "*",
"vue": "^3.0.0"
},
"engines": {
Expand Down
102 changes: 72 additions & 30 deletions code/renderers/vue3/src/decorateStory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,56 +14,98 @@ import type { VueRenderer } from './types';
function normalizeFunctionalComponent(options: ConcreteComponent): ComponentOptions {
return typeof options === 'function' ? { render: options, name: options.name } : options;
}

function prepare(
rawStory: VueRenderer['storyResult'],
innerStory?: ConcreteComponent
): Component | null {
const story = rawStory as ComponentOptions;
if (story === null) {
/**
* This function takes a rawStory (as returned by the storyFn), and an innerStory (optional),
* which is used to render child components and returns a Component object.
* If an innerStory is provided, it merges the components object of the story with the components object for the innerStory.
*
* @param {VueRenderer['storyResult']} rawStory - The rawStory returned by the storyFn.
* @param {ConcreteComponent} innerStory - Optional innerStory used for rendering child components.
* @returns {Component | null} - Returns a Component object that can be rendered.
*/
function prepare(story: VueRenderer['storyResult'], innerStory?: Component): Component | null {
if (!story) {
return null;
}
if (typeof story === 'function') return story; // we don't need to wrap a functional component nor to convert it to a component options

// If story is already a function, we don't need to wrap it nor convert it
if (typeof story === 'function') return story;

// Normalize the functional component, and make sure inheritAttrs is set to false
const normalizedStory = { ...normalizeFunctionalComponent(story), inheritAttrs: false };
Comment on lines +34 to +35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this disable attribute fallthough on all the prepared stories including decorated onces,


// If an innerStory is provided, merge its components object with story.components
if (innerStory) {
return {
// Normalize so we can always spread an object
...normalizeFunctionalComponent(story),
...normalizedStory,
components: { ...(story.components || {}), story: innerStory },
};
}

// Return a Component object with a render function that returns the normalizedStory
return {
render() {
return h(story);
return h(normalizedStory);
},
};
}

/**
* This function takes a storyFn and an array of decorators, and returns a decorated version of the input storyFn.
*
* @param {LegacyStoryFn<VueRenderer>} storyFn - The story function to decorate.
* @param {DecoratorFunction<VueRenderer>[]} decorators - The array of decorators to apply to the story function.
* @returns {LegacyStoryFn<VueRenderer>} - Returns a decorated version of the input storyFn.
*/
export function decorateStory(
storyFn: LegacyStoryFn<VueRenderer>,
decorators: DecoratorFunction<VueRenderer>[]
): LegacyStoryFn<VueRenderer> {
return decorators.reduce(
(decorated: LegacyStoryFn<VueRenderer>, decorator) => (context: StoryContext<VueRenderer>) => {
let story: VueRenderer['storyResult'] | undefined;
/**
* This function receives two arguments: accuDecoratedStoryFn (the accumulator for the decorated story function) and currentDecoratorFn (the current decorator function).
* It applies the decorator to the accuDecoratedStoryFn by returning a new decorated storyFn from the currentDecoratedStory function.
*
* @param {LegacyStoryFn<VueRenderer>} accuDecoratedStoryFn - The accumulator of the decorated story function.
* @param {DecoratorFunction<VueRenderer>} currentDecoratorFn - The current decorator function.
*/
const finalDecoratedStoryFn = decorators.reduce((accuDecoratedStoryFn, currentDecoratorFn) => {
Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen @tmeasday would you mind taking a look? Thanks!

let storyResult: VueRenderer['storyResult'];

const decoratedStory: VueRenderer['storyResult'] = decorator((update) => {
const sanitizedUpdate = sanitizeStoryContextUpdate(update);
// update the args in a reactive way
if (update) sanitizedUpdate.args = Object.assign(context.args, sanitizedUpdate.args);
story = decorated({ ...context, ...sanitizedUpdate });
return story;
/**
* This function receives a context argument (the current context of the story function),
* and returns a decorated version of the story function.
*
* @param {StoryContext<VueRenderer>} context - The current context of the story function.
*/
const currentDecoratedStory = (context: StoryContext<VueRenderer>) =>
currentDecoratorFn((update) => {
const mergedContext = { ...context, ...sanitizeStoryContextUpdate(update) };
storyResult = accuDecoratedStoryFn(mergedContext);
context.args = mergedContext.args;
return storyResult;
}, context);

if (!story) story = decorated(context);

if (decoratedStory === story) {
return story;
}
/**
* This function receives a context argument (the current context of the story function),
* and returns the final decorated story function with all the decorators applied
* @param {StoryContext<VueRenderer>} context - The current context of the story function.
* @returns {VueRenderer['storyResult']} - Returns the final decorated story function with all the decorators applied.
*/
return (context: StoryContext<VueRenderer>) => {
const story = currentDecoratedStory(context);
if (!storyResult) storyResult = accuDecoratedStoryFn(context);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this line mean the later decorators will get called even if the earlier decorator (currentDecoratorFn) never calls the passed storyFn (defined on line 81)?

As an aside it would help me at least (maybe it's obvious to others) to explain what's happening here -- why do we return storyResult sometimes, and other times we call prepare() on it?

Copy link
Contributor Author

@chakAs3 chakAs3 Jun 13, 2023

Choose a reason for hiding this comment

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

exactly @tmeasday if you don't get a deccorated Story from the decorator or the decorator did you call storyFn, we just take the last one. so we can now have decorator that returns null, so we kind of skip them.
so now we can have

(storyFn: PartialStoryFn, context: StoryContext) => context.args.condition? storyFn(): null

Copy link
Contributor Author

@chakAs3 chakAs3 Jun 13, 2023

Choose a reason for hiding this comment

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

we return storyResult sometimes, and other times we call prepare() on it?

we don't need to prepare it because the previous one is already prepared we just return 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.

we can have decorators that have just side effects and not touching the rendering Tree. by doing something like this.

(storyFn: PartialStoryFn, context: StoryContext) => {
       saveData(context.args.data)
      return null
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an aside it would help me at least (maybe it's obvious to others)

😄 I think you are the only one or at least to first one who spotted the Key change in this PR that fix the conditional decorator issue

Copy link
Member

Choose a reason for hiding this comment

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

we can have decorators that have just side effects and not touching the rendering Tree. by doing something like this.

I'm confused. So a decorator that returns null would have "no effect" on rendering, in the sense that later decorators and the story function would still render? That's inconsistent with the other frameworks, and I think the opposite of what we are trying to achieve (consistency in decorator behaviour).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no of course the children won't render since the parent not providing any story, one thing is important I'm sure you know it is rendering should happen down -> up the tree.

decorators.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what I meant no effect on the parent so only if put it like last to run

decorators: [
    // conditional decorator, runs before last
    (storyFn: PartialStoryFn, context: StoryContext) => {
      return context.args.condition
        ? storyFn()
        : null;
    },
    // decorator A that uses hooks
    (storyFn: PartialStoryFn, context: StoryContext) => {
      useEffect(() => {});

      return storyFn({ args: { ...context.args, text: `WrapperA( ${context.args['text']} )` } });
    },
    // decorator B that uses hooks
    (storyFn: PartialStoryFn, context: StoryContext) => {
      useEffect(() => {});

      return storyFn({ args: { ...context.args, text: `WrapperB( ${context.args['text']} )` } });
    },
    
  ],

in this case, won't affect the rendered tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a repo on Stackblitz. if you want to test that fast without spawning any sandbox https://stackblitz.com/~/github.com/chakAs3/storybook-vue3-decorators

if (!story) return storyResult;
if (story === storyResult) return storyResult;

const innerStory = () => h(story!);
return prepare(decoratedStory, innerStory) as VueRenderer['storyResult'];
},
(context) => prepare(storyFn(context)) as LegacyStoryFn<VueRenderer>
);
return prepare(story, () => h(storyResult, context.args)) as VueRenderer['storyResult'];
};
}, storyFn);
/**
* This function receives a context argument (the current context of the story function), and returns the final decorated story function with all the decorators applied.
*
* @param {StoryContext<VueRenderer>} context - The current context of the story function.
* @returns {LegacyStoryFn<VueRenderer>} - Returns the final decorated story function with all the decorators applied.
*/
return (context: StoryContext<VueRenderer>) =>
prepare(finalDecoratedStoryFn(context)) as LegacyStoryFn<VueRenderer>;
}
4 changes: 2 additions & 2 deletions code/renderers/vue3/src/docs/sourceDecorator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ function getTemplateComponents(
if (!template) return [h(story, context?.args)];
return getComponents(template);
} catch (e) {
console.log('error', e);
// console.log('error', e);
return [];
}
}
Expand Down Expand Up @@ -248,7 +248,7 @@ export function generateTemplateSource(
.map((child) => child.content)
.join('')
: '';
console.log(' vnode ', vnode, ' childSources ', childSources, ' attributes ', attributes);

const name =
typeof type === 'string'
? type
Expand Down
41 changes: 20 additions & 21 deletions code/renderers/vue3/src/render.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
/* eslint-disable no-param-reassign */
import type { App } from 'vue';
import { createApp, h, reactive, isVNode, isReactive } from 'vue';
import type { ArgsStoryFn, RenderContext } from '@storybook/types';
import type { App, ConcreteComponent } from 'vue';
import { createApp, h, reactive, isReactive } from 'vue';
import type { RenderContext, ArgsStoryFn } from '@storybook/types';
import type { Args, StoryContext } from '@storybook/csf';

import type { StoryFnVueReturnType, StoryID, VueRenderer } from './types';
import { cloneDeep } from 'lodash';
import type { VueRenderer, StoryID } from './types';

const slotsMap = new Map<
StoryID,
{
component?: Omit<ConcreteComponent<any>, 'props'>;
reactiveSlots?: Args;
}
>();

chakAs3 marked this conversation as resolved.
Show resolved Hide resolved
export const render: ArgsStoryFn<VueRenderer> = (props, context) => {
const { id, component: Component } = context;
Expand All @@ -14,7 +23,7 @@ export const render: ArgsStoryFn<VueRenderer> = (props, context) => {
);
}

return () => h(Component, props, generateSlots(context));
return h(Component, props, generateSlots(context));
chakAs3 marked this conversation as resolved.
Show resolved Hide resolved
};

// set of setup functions that will be called when story is created
Expand Down Expand Up @@ -51,10 +60,9 @@ export function renderToCanvas(
// normally storyFn should be call once only in setup function,but because the nature of react and how storybook rendering the decorators
// we need to call here to run the decorators again
// i may wrap each decorator in memoized function to avoid calling it if the args are not changed
const element = storyFn(); // call the story function to get the root element with all the decorators
const args = getArgs(element, storyContext); // get args in case they are altered by decorators otherwise use the args from the context
storyFn(); // call the story function to get the root element with all the decorators

updateArgs(existingApp.reactiveArgs, args);
updateArgs(existingApp.reactiveArgs, storyContext.args);
return () => {
teardown(existingApp.vueApp, canvasElement);
};
Expand All @@ -66,17 +74,17 @@ export function renderToCanvas(
setup() {
storyContext.args = reactive(storyContext.args);
const rootElement = storyFn(); // call the story function to get the root element with all the decorators
const args = getArgs(rootElement, storyContext); // get args in case they are altered by decorators otherwise use the args from the context

const appState = {
vueApp,
reactiveArgs: reactive(args),
reactiveArgs: reactive(storyContext.args),
};
map.set(canvasElement, appState);

return () => {
// not passing args here as props
// treat the rootElement as a component without props
return h(rootElement);
return h(rootElement, appState.reactiveArgs);
};
},
});
Expand Down Expand Up @@ -107,15 +115,6 @@ function generateSlots(context: StoryContext<VueRenderer, Args>) {

return reactive(Object.fromEntries(slots));
}
/**
* get the args from the root element props if it is a vnode otherwise from the context
* @param element is the root element of the story
* @param storyContext is the story context
*/

function getArgs(element: StoryFnVueReturnType, storyContext: StoryContext<VueRenderer, Args>) {
return element.props && isVNode(element) ? element.props : storyContext.args;
}

/**
* update the reactive args
Expand All @@ -133,7 +132,7 @@ export function updateArgs(reactiveArgs: Args, nextArgs: Args) {
}
});
// update currentArgs with nextArgs
Object.assign(currentArgs, nextArgs);
Object.assign(currentArgs, cloneDeep(nextArgs));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ const meta = {
// storybook render function is not a functional component. it returns a functional component or a component options
render: (args) => {
// create the slot contents as a functional components
const header = ({ title }: { title: string }) => h('h3', `${args.header} - Title: ${title}`);
const header = ({ title }: { title: string }) => h('h4', `${args.header} - Title: ${title}`);
const defaultSlot = () => h('p', `${args.default}`);
const footer = () => h('p', `${args.footer}`);
// vue render function is a functional components
return () =>
h('div', [
`Custom render uses a functional component, and passes slots to the component:`,
h('h3', 'Custom render returns a functional component'),
h(Reactivity, args, { header, default: defaultSlot, footer }),
]);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ const meta = {
components: {
Reactivity,
},
template: `<div>Custom render uses options api and binds args to data:
template: `<div>
<h3> Custom render returns options API Component</h3>
<Reactivity v-bind="args">
<template #header="{title}"><h3>{{ args.header }} - Title: {{ title }}</h3></template>
<template #header="{title}"><h4>{{ args.header }} - Title: {{ title }}</h4></template>
<template #default>{{ args.default }}</template>
<template #footer>{{ args.footer }} </template>
</Reactivity>
Expand Down
Loading