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

Vue3: Fix reactive args updates in decorators #22717

Merged
merged 4 commits into from
May 25, 2023
Merged

Vue3: Fix reactive args updates in decorators #22717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d0c654
Fix hooks story, by updating args reactively in decorators
kasperpeulen May 24, 2023
e4dafb9
Make sure args are non-reactive in source decorator
kasperpeulen May 24, 2023
6789283
Make sure to copy args in story as Vue mutates the decorator update (…
kasperpeulen May 24, 2023
0e63533
Avoid circular objects in Vue
kasperpeulen May 24, 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
3 changes: 2 additions & 1 deletion code/addons/controls/template/stories/basics.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { PartialStoryFn, StoryContext } from '@storybook/types';
export default {
component: globalThis.Components.Pre,
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => storyFn({ args: { object: context.args } }),
(storyFn: PartialStoryFn, context: StoryContext) =>
storyFn({ args: { object: { ...context.args } } }),
Copy link
Member

Choose a reason for hiding this comment

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

So the problem is assigning args.object = args blows up stringify? I guess what we are doing in these decorators is pretty bizarre so having to do hacks is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is right, it blows up stringify!
Okay, so then I just do it like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kasperpeulen @tmeasday it is very weird to impose spreading args storyFn({ args: { object: { ...context.args } } }) in order for it to work,

Copy link
Member

Choose a reason for hiding this comment

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

@chakAs3 what we are doing in these stories (assigning the whole args object into a single arg) is very weird, so I wouldn't worry about it (from a user perspective).

Copy link
Contributor

@chakAs3 chakAs3 Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes i know i dealt with it when I was rewriting vue renderer, it was weird for me the first time but after that I found it legit and even useful, to alter the story's args, using the decorator that can cherry pick some args and passe them to single object prop for the component should work

  // Compose the set of  args into `object`, so the pre component only needs a single prop
  //   (selecting only the args specified on parameters.argNames if set)
  decorators: [
    (storyFn: PartialStoryFn, context: StoryContext) => {
      const { argNames } = context.parameters;
      const object = argNames ? pick(context.args, argNames) : context.args;
      return storyFn({ args: { object } });
    },
  ],

unless we are doing something wrong in the current Vue implementation

Copy link
Contributor

@chakAs3 chakAs3 Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@tmeasday check https://stackblitz.com/~/github.com/storybookjs/vue3-code-examples,
I have included all these cases and it just working fine. you don't have to spread your context.args

image

Copy link
Member

Choose a reason for hiding this comment

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

@chakAs3 has something changed to make it no longer necessary? @kasperpeulen added the spreading because this change mean the args object became self-referential in these stories and thus created problems when it was serialized over the channel. At the time I played with it a bit and saw the problem too.

image

],
argTypes: {
boolean: { control: 'boolean' },
Expand Down
3 changes: 2 additions & 1 deletion code/addons/controls/template/stories/conditional.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { PartialStoryFn, StoryContext } from '@storybook/types';
export default {
component: globalThis.Components.Pre,
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => storyFn({ args: { object: context.args } }),
(storyFn: PartialStoryFn, context: StoryContext) =>
storyFn({ args: { object: { ...context.args } } }),
],
};

Expand Down
3 changes: 2 additions & 1 deletion code/addons/controls/template/stories/disable.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { PartialStoryFn, StoryContext } from '@storybook/types';
export default {
component: globalThis.Components.Pre,
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => storyFn({ args: { object: context.args } }),
(storyFn: PartialStoryFn, context: StoryContext) =>
storyFn({ args: { object: { ...context.args } } }),
],
};

Expand Down
3 changes: 2 additions & 1 deletion code/addons/controls/template/stories/filters.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { PartialStoryFn, StoryContext } from '@storybook/types';
export default {
component: globalThis.Components.Pre,
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => storyFn({ args: { object: context.args } }),
(storyFn: PartialStoryFn, context: StoryContext) =>
storyFn({ args: { object: { ...context.args } } }),
],
args: {
helloWorld: 1,
Expand Down
3 changes: 2 additions & 1 deletion code/addons/controls/template/stories/issues.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { PartialStoryFn, StoryContext } from '@storybook/types';
export default {
component: globalThis.Components.Pre,
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => storyFn({ args: { object: context.args } }),
(storyFn: PartialStoryFn, context: StoryContext) =>
storyFn({ args: { object: { ...context.args } } }),
],
};

Expand Down
3 changes: 2 additions & 1 deletion code/addons/controls/template/stories/matchers.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { PartialStoryFn, StoryContext } from '@storybook/types';
export default {
component: globalThis.Components.Pre,
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => storyFn({ args: { object: context.args } }),
(storyFn: PartialStoryFn, context: StoryContext) =>
storyFn({ args: { object: { ...context.args } } }),
],
};

Expand Down
3 changes: 2 additions & 1 deletion code/addons/controls/template/stories/sorting.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { PartialStoryFn, StoryContext } from '@storybook/types';
export default {
component: globalThis.Components.Pre,
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => storyFn({ args: { object: context.args } }),
(storyFn: PartialStoryFn, context: StoryContext) =>
storyFn({ args: { object: { ...context.args } } }),
],
argTypes: {
x: { type: { required: true } },
Expand Down
2 changes: 1 addition & 1 deletion code/lib/store/template/stories/argMapping.stories.ts
View file
Open in desktop
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/store/template/stories/argTypes.stories.ts
View file
Open in desktop
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: 2 additions & 1 deletion code/lib/store/template/stories/args.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ export default {
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => {
const { argNames } = context.parameters;
const object = argNames ? pick(context.args, argNames) : context.args;
const args = { ...context.args };
const object = argNames ? pick(args, argNames) : args;
return storyFn({ args: { object } });
},
],
Expand Down
8 changes: 4 additions & 4 deletions code/renderers/vue3/src/decorateStory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ export function decorateStory(
let story: VueRenderer['storyResult'] | undefined;

const decoratedStory: VueRenderer['storyResult'] = decorator((update) => {
story = decorated({
...context,
...sanitizeStoryContextUpdate(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;
}, context);

Expand Down
3 changes: 2 additions & 1 deletion code/renderers/vue3/src/docs/sourceDecorator.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

toRaw return the original object from proxies created by reactive().
and destructuring _args already kills the reactivity so i don't see the point.
Anyway i'm changing the sourceDecorator implementation, so it is ok for now. toRaw gives no reactive object

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 think the problem was that {...args} only removes the reactivity one level deep.
And not when the args contain objects as well.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import parserHTML from 'prettier/parser-html';

// eslint-disable-next-line import/no-extraneous-dependencies
import { isArray } from '@vue/shared';
import { toRaw } from 'vue';

type ArgEntries = [string, any][];
type Attribute = {
Expand Down Expand Up @@ -60,7 +61,7 @@ function getComponentNameAndChildren(component: any): {
*/
function generateAttributesSource(_args: Args, argTypes: ArgTypes, byRef?: boolean): string {
// create a copy of the args object to avoid modifying the original
const args = { ..._args };
const args = { ...toRaw(_args) };
// filter out keys that are children or slots, and convert event keys to the proper format
const argsKeys = Object.keys(args)
.filter(
Expand Down