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/17063/avoid undefined args #17973

Closed
wants to merge 274 commits into from
Closed

Fix/17063/avoid undefined args #17973

wants to merge 274 commits into from

Conversation

evont
Copy link

@evont evont commented Apr 15, 2022

Issue: #17063

What I did

fix args values not present in url are casted to !undefined for controls of type select and radio issue

How to test

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

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

shilman and others added 30 commits December 1, 2021 12:37
Preview: Don't hide the story while preparing
…-no-inline

Don't render with `modernInline` if `inlineStories` is `false`
…automigrate

CLI: Fix mainjsFramework automigrate
…ion-fix

Addon-docs: Fix transclusion crash on webpack rules without test field
Angular: Fix tsConfig paths not resolving for Angular >=12.2
@nx-cloud
Copy link

nx-cloud bot commented Apr 15, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 471fb5c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link
Contributor

@kylegach kylegach left a comment

Choose a reason for hiding this comment

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

Thank for fixing this important issue, @evont! And with your very first contribution. 👏

@shilman @tmeasday — This change seems sound to me, but the args code in general is likely quite nuanced and I think it would benefit from you taking a look as well. Also, is it OK that this PR is against main instead of next?

@kylegach
Copy link
Contributor

kylegach commented May 3, 2022

@evont — I'd do this myself, but I cannot push to your fork's branch. Can you please add this story, to the examples/official-storybook/stories/core/args.stories.js file?

export const TestUndefinedArgs = Template.bind({});
TestUndefinedArgs.args = {
  first: 'Bob',
  last: 'Miller',
  foo: 'bar',
};
TestUndefinedArgs.argTypes = {
  first: {
    options: ['Bob', 'Alice'],
  },
  last: {
    options: ['Miller', 'Meyer'],
  },
  foo: {
    control: { type: 'text' },
  },
};

That will allow maintainers to easily test your change (use the same repro steps as the linked issue) in the deployed Storybook. Thanks!

@shilman shilman changed the base branch from main to next May 4, 2022 01:35
@shilman shilman changed the base branch from next to main May 4, 2022 01:35
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 @evont @kylegach -- thanks for the fix! Indeed all PRs should go against next not main unless it's a special case, which AFAICT this is not. Please rebase & retarget the PR. 🙏

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.

Thanks for the contribution @evont. Can you please add test case(s) that demonstrate the original problem?

@@ -101,7 +101,8 @@ export const validateOptions = (args: Args, argTypes: ArgTypes): Args => {
const isValidArray = isArray && invalidIndex === -1;

if (args[key] === undefined || options.includes(args[key]) || isValidArray) {
Copy link
Member

Choose a reason for hiding this comment

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

This check for args[key] === undefined seems a little pointless if we later do if (key in args). Do we understand why the code was there in the first place? We should get rid of it, and perhaps figure that out.

Copy link
Member

Choose a reason for hiding this comment

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

Or does it remain here to avoid the warning lower down?

Copy link
Author

Choose a reason for hiding this comment

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

key in args is not the same as the check for args[key] === undefined, caused if the key is existed in args, but is set as undefined, it shall pass the if condition. In the other hand, if the key is not existed in args, the value will still be undefined, then it will pollute the acc and turn out the param will become undefined

@evont evont changed the base branch from main to next May 4, 2022 13:58
@evont
Copy link
Author

evont commented May 4, 2022

@evont — I'd do this myself, but I cannot push to your fork's branch. Can you please add this story, to the examples/official-storybook/stories/core/args.stories.js file?

export const TestUndefinedArgs = Template.bind({});
TestUndefinedArgs.args = {
  first: 'Bob',
  last: 'Miller',
  foo: 'bar',
};
TestUndefinedArgs.argTypes = {
  first: {
    options: ['Bob', 'Alice'],
  },
  last: {
    options: ['Miller', 'Meyer'],
  },
  foo: {
    control: { type: 'text' },
  },
};

That will allow maintainers to easily test your change (use the same repro steps as the linked issue) in the deployed Storybook. Thanks!

Sure, with pleasure, I have added it and push it to the branch, except I added control: { type: 'select' } because it is odd that if I don't do that, the options will not transform into select but text

@evont evont changed the base branch from next to main May 4, 2022 14:52
@evont evont changed the base branch from main to next May 4, 2022 14:54
@evont
Copy link
Author

evont commented May 4, 2022

Sorry for my mistake, fork from main branch and rebase to next will be that tough, so I decided to fork form next and make a new PR, please check in this new PR
@tmeasday @kylegach @shilman

@shilman
Copy link
Member

shilman commented May 5, 2022

Thanks @evont! Closing this in favor of #18135

@shilman shilman closed this May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants