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

Controls: Don't set arg in validateOptions if it would be undefined #15654

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

tmeasday
Copy link
Member

Fixes #15630

Issue:

Code set args[X] = undefined if it wasn't already defined. This was made much more obvious by the removal of default values from args.

What I did

Ensure the args has the key.

How to test

  • Is this testable with Jest or Chromatic screenshots?
    Yes, Jest test.

@tmeasday tmeasday added bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Jul 22, 2021
@tmeasday tmeasday requested review from ghengeveld and shilman July 22, 2021 11:30
@nx-cloud
Copy link

nx-cloud bot commented Jul 22, 2021

Nx Cloud Report

CI ran the following commands for commit 039eeda. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@nx-cloud
Copy link

nx-cloud bot commented Jul 22, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit 039eeda.
Please make sure you set the \ NX_BRANCH\ environment variable in your CI pipeline .

Check the Getting started section to configure the app.


Sent with 💌 from NxCloud.

@@ -72,7 +72,9 @@ export const combineArgs = (value: any, update: any): Args => {
export const validateOptions = (args: Args, argTypes: ArgTypes): Args => {
return Object.entries(argTypes).reduce((acc, [key, { options }]) => {
if (!options) {
acc[key] = args[key];
if (key in args) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We could also use args[key] !== undefined, or various other techniques. I honestly don't know what's the right way.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I think this is fine and better than checking against undefined. The alternative would be hasOwnProperty, but since we're dealing with plain objects, it doesn't matter.

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.

💯

@shilman shilman changed the title Don't set arg in validateOptions if it would be undefined. Controls: Don't set arg in validateOptions if it would be undefined Jul 22, 2021
@shilman shilman merged commit 2e83d6b into next Jul 22, 2021
@shilman shilman deleted the 15630-undefined-defaults branch July 22, 2021 13:07
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jul 22, 2021
shilman added a commit that referenced this pull request Jul 22, 2021
Controls: Don't set arg in validateOptions if it would be `undefined`
@kylegach
Copy link
Contributor

Thank you for fixing this (and back-porting to 6.3)! I was able to remove a bunch of decorators working around this issue. 💥

@shilman shilman added this to the 6.4 PRs milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: controls 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storybook controls shouldn't pass 'undefined' by default
4 participants