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 reactive args updates in decorators #22717

Merged
merged 4 commits into from
May 25, 2023

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented May 24, 2023

What I did

When doing a decorator update with args, we used to overwrite the reactive args, that are made reactive in the renderToCanvas function. This PR makes sure we don't overwrite those args, but mutate them instead.

We test those decorator args updates in the hooks chromatic stories. I found out that they are accidentally accepted with a visual bug on next, when back porting a PR to main:
#22692

With this PR, args update should work again in Vue. At least the chromatic test are working again.

How to test

See the lib/store/hooks stories in chromatic.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@shilman shilman changed the title Vue3: Make sure that args are updated reactively in decorator updates Vue3: Fix reactive args updates in decorators May 24, 2023
@kasperpeulen kasperpeulen requested a review from tmeasday May 24, 2023 12:19
@kasperpeulen kasperpeulen added the patch:yes Bugfix & documentation PR that need to be picked to main branch label May 24, 2023
@kasperpeulen kasperpeulen requested a review from chakAs3 May 24, 2023 13:08
Copy link
Contributor

@chakAs3 chakAs3 left a comment

Choose a reason for hiding this comment

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

I'm testing this, i don't really like use destructuring unless we really need it.
I know that pointing the same reference may cause some problem.

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.

@kasperpeulen
Copy link
Contributor Author

@chakAs3 Yeah, I don't like the spreading everywhere as well, but otherwise we get an infinite JSON structure 😅

@chakAs3
Copy link
Contributor

chakAs3 commented May 24, 2023

@chakAs3 Yeah, I don't like the spreading everywhere as well, but otherwise we get an infinite JSON structure 😅

Yes i know I may have a solution let me try it.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

This seems like the right approach, although I am not sure of the details on how to update the reactive args object.

@@ -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

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

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

@kasperpeulen kasperpeulen merged commit 09db9f7 into next May 25, 2023
@kasperpeulen kasperpeulen deleted the kasper/update-args-reactively branch May 25, 2023 08:06
kasperpeulen added a commit that referenced this pull request May 25, 2023
Vue3: Fix reactive args updates in decorators
kasperpeulen added a commit that referenced this pull request May 25, 2023
Vue3: Fix reactive args updates in decorators
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 26, 2023
shilman pushed a commit that referenced this pull request May 26, 2023
Vue3: Fix reactive args updates in decorators
@shilman shilman mentioned this pull request May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
args bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch vue3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants