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

Fix Vue prop defaults for objects/arrays. #12634 #12795

Closed
wants to merge 1 commit into from

Conversation

jgaehring
Copy link

@jgaehring jgaehring commented Oct 16, 2020

Issue: #12634

What I did

Added a check for whether or not an object/array prop's default value is a factory function, as prescribed by the Vue docs. See #12634 for more details. I also added a story to vue-kitchen-sink for testing. I'm not altogether sure how to add a Jest test for the extractArgTypes function, since I don't feel I fully understand its usage, but am willing to try with some guidance.

Also note that I included 'other' among the types I'm checking against. I tried to restrict this to just 'object' and 'array', but there is a peculiarity in how docgen works, where if the component's type is specified as [Boolean, Array], it gets classified as 'other'. Sorry, that's not a great explanation, but I'm happy to discuss further if it's of concern.

How to test

See the new Varied story in examples/vue-kitchen-sink/src/stories/components/button.stories.js. Previously, this would have produced a warning and broken the component, as described in #12634.

  • Is this testable with Jest or Chromatic screenshots?
    Maybe?
  • Does this need a new example in the kitchen sink apps?
    One is provided.
  • Does this need an update to the documentation?
    No.

If your answer is yes to any of these, please make sure to include it in your PR.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Hi @jgaehring! Thanks for the fix. The code looks good, and it's working great in the browser. However storyshots is failing. I took a quick look but the fix wasn't immediately obvious. Can you please take a look?

  ● Storyshots › Button › Square

    error: [Vue warn]: Error in render: "Error: error: [Vue warn]: Property or method "corners" is not defined on the instance but referenced during render. Make sure that this property is reactive, either in the data option, or for class-based components, by initializing the property. See: https://vuejs.org/v2/guide/reactivity.html#Declaring-Reactive-Properties.

@jgaehring
Copy link
Author

Ah, ok, yes, I have a good guess what this could be and will take a closer look later today.

@bhaidar
Copy link

bhaidar commented Dec 23, 2020

@jgaehring Hello,

Any idea when this will be shipped? For now, I am providing the props even if it has a default value just not to break the storybook browser.

Thank you
Bilal

@jgaehring
Copy link
Author

Ack! So sorry everyone, I was totally swept up in other issues the last couple month and lost track of this one. I can at least get this PR fixed up so it's mergable, though no idea if/when it would be merged after that point. Might be tough w/ holidays up on us now, but first thing in the new year at the latest.

@jgaehring
Copy link
Author

I'm having some trouble tracking down exactly where this error is originating from. It seems like the changes I made in addons/docs/src/frameworks/vue/extractArgTypes.ts aren't effecting the how args are picked up and rendered by the snapshot tests. I'm struggling to understand the flow of execution, but I suspect I need to apply a similar change in addons/storyshots/storyshots-core/src/frameworks/vue/ or somewhere thereabouts.

@shilman
Copy link
Member

shilman commented Jul 19, 2021

This PR has been obviated by subsequent changes to how we deal with argTypes: #14900

Sorry for letting this PR get lost in the shuffle!! ☹️

@shilman shilman closed this Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants