Check for truthiness vs null before spreading attributes

[JamesIves]

When you pass in args into the default export of a story any attribute that does not have a defined default value ends up getting treated as an empty string (""), which in turn ends up getting spread onto the component. As a result every possible string attribute that can exist ends up showing up in the docs view. As a consumer I would expect only those that have a defined default value that is setup in the controls tab would show up here as opposed to empty strings for everything, as it keeps the documentation more clear.

For example, here's a property that was problematic which doesn't have a default value. Because we're using the ifDefined directive on a video element in the render method, passing in an empty string ends up breaking this in the docs preview as it's no longer of undefined|null type.

@property({type: String})
src;

@property({type: String})
crossorigin;

@property({type: String})
title = 'player'

On my component it was rendering like so:

<!-- Unexpected -->
<my-component src="" crossorigin="" title="player"></my-component>

<!-- Expected -->
<my-component title="player"></my-component>

Which based on the following results in:

<!-- Lit -->
<video crossorigin="${ifDefined(this.crossorigin)}" src="${ifDefined(src)}" title="player"></video>

<!-- Rendered -->
<video crossorigin="" src="" title="player"></video>

To solve this I've adjusted the spread function to check for truthiness as opposed to a null type check which seems to do the trick, for my use case at least. With this change only defined things in the controls get applied to the component. The alternative would be to update every instance of ifDefined to fallback to a undefined|null value, ie ifDefined(this.src || undefined), but I personally find it odd that everything shows up as empty strings in the code previews.

Let me know if this fix makes sense to you, or if there's anything else you think would need changing, there may be some unintended side effects I haven't thought through with this. I wanted to add unit tests but there wasn't a preexisting fixture setup to validate the directives.

[JamesIves]

Another option would be to keep the following as undefined also;

https://github.com/break-stuff/wc-storybook-helpers/blob/main/src/storybook-utils.ts#L85-L87

function getArgs(component?: Declaration): Record<string, any> {
  const args = Object.entries(getArgTypes(component))
    // We only want to get args that have a control in Storybook
    .filter(([, value]) => value?.control)
    .map(([key, value]) => {
      const defaultValue = getDefaultValue(value.defaultValue);
      return {
        [key]: defaultValue,
      };
    })
    .reduce((acc, value) => ({ ...acc, ...value }), {});
  return args;
}

[break-stuff]

It looks like the attributes are still being added for me when there is no value.

[JamesIves]

It looks like the attributes are still being added for me when there is no value.

Interesting, I applied this fix locally by directly modifying spread.js within node_modules and restarted Storybook, and it seems to prevent things being spread with no value.

For example before, I was seeing this <my-component title=""></my-component> for an empty string value where as now I see <my-component></my-component> until I add something in the controls. It applies both in the DOM and in the code preview Storybook generates.

[JamesIves]

Did some more testing on this today to be sure I wasn't seeing things, also tested out the props in the query params:

With text as '':

&args=text:!null&globals=cssVariables:!undefined

---

With defined text:

&args=text:Callout+Text&globals=cssVariables:!undefined

---

With Boolean as false:

&args=text:Callout+Text;is-compact:!false&globals=cssVariables:!undefined