when testing with storyshots, args with a null value entry break the story loading chain

[chadi-kazan]

Describe the bug
using @storybook/react v6.0.21
When running image snapshot tests, storybook fails to load a story with a args parameter entry with a null value, even though the component's type for that prop takes null.
The story renders properly in the storybook UI though

To Reproduce
Steps to reproduce the behavior:

1. testing the ProgressBar component from kendo ui
2. with the story below

import { ProgressBar, ProgressBarProps } from "@progress/kendo-react-progressbars";
import { Story } from "@storybook/react/types-6-0";
import React from "react";

export default {
	title: "Components/Progress Bars/ProgressBar",
	component: ProgressBar,
};

const Example: Story<ProgressBarProps> = (props) => <ProgressBar min={0} max={100} value={66} {...props} />;

/*storybook does not understand the null args.
this breaks the story and therefore storybook ignores all the stories behind it in the pecking order when running snapshot tests*/

export const Indeterminate = Example.bind({});
Indeterminate.args = {
	value: null, // <----------------------------- culprit is this
	style: { height: "4px" },
};

Mind you that ProgressBar declares a number|null value prop

Expected behavior
story to be picked up and test run.

System:
Environment Info:

  System:
    OS: Windows 10 10.0.17763
    CPU: (8) x64 Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz
  Binaries:
    Node: 12.18.0 - C:\Program Files\nodejs\node.EXE
    npm: 6.14.8 - D:\GIT\vitol-design-system\node_modules\.bin\npm.CMD
  Browsers:
    Chrome: 84.0.4147.125
    Edge: Spartan (44.17763.831.0)
  npmPackages:
    @storybook/addon-actions: 6.0.21 => 6.0.21
    @storybook/addon-essentials: 6.0.21 => 6.0.21
    @storybook/addon-links: 6.0.21 => 6.0.21
    @storybook/addon-storyshots: 6.0.21 => 6.0.21
    @storybook/addon-storyshots-puppeteer: 6.0.21 => 6.0.21
    @storybook/addon-viewport: 6.0.21 => 6.0.21
    @storybook/addons: 6.0.21 => 6.0.21
    @storybook/node-logger: 6.0.21 => 6.0.21
    @storybook/preset-create-react-app: 3.1.4 => 3.1.4
    @storybook/react: 6.0.21 => 6.0.21

[Slioniwe]

The same issue for undefined.

[bogdanorzea]

I've been bashing my head against the keyboard for more than a week and just found out that the null and undefined will break the whole jest storyshot generation. After removing the null arg, it started working.

[thibaudcolas]

I’m not sure if this is clear enough from the above:

• This affects all Storyshots usage, not just image snapshots, not just React
• It’s not just the affected stories that fail to run – as soon as this bug occurs, Storyshots stops running any further tests
• This all happens silently. There is no indication from the tests output that anything is broken

To me the fact that this is all happening while the test suite still passes is much more problematic than those few stories not working as expected.

The error is:

Cannot read property 'defaultValue' of undefined

TypeError: Cannot read property 'defaultValue' of undefined
    at storyshots-null-args/node_modules/@storybook/client-api/dist/story_store.js:644:38
    at Array.reduce (<anonymous>)
    at StoryStore.addStory (storyshots-null-args/node_modules/@storybook/client-api/dist/story_store.js:641:50)
    at Object.api.add (storyshots-null-args/node_modules/@storybook/client-api/dist/client_a…/storyshots-null-args/node_modules/jest-jasmine2/build/index.js:60:19)
    at storyshots-null-args/node_modules/jest-runner/build/runTest.js:385:24
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (storyshots-null-args/node_modules/jest-runner/build/runTest.js:161:24)
    at _next (storyshots-null-args/node_modules/jest-runner/build/runTest.js:181:9)

It’s raised here, as part of the array destructuring, because the expected argType is undefined:

storybook/lib/client-api/src/story_store.ts

Lines 413 to 415 in 748710c

	const defaultArgs: Args = Object.entries(
	argTypes as Record<string, { defaultValue: any }>
	).reduce((acc, [arg, { defaultValue }]) => {

It gets swallowed by the catch here:

storybook/lib/client-api/src/config_api.ts

Lines 18 to 20 in 748710c

	} catch (err) {
	this._storyStore.setError(err);
	}

---

As far as I can see, this undefined comes from one of the _argTypesEnhancers. It’s not clear to me whether the bug is with one of those enhancers (ensureArgTypes, inferArgTypes), or whether the code above should handle argType being undefined.

Without understanding it all,

storybook/lib/client-api/src/story_store.ts

Lines 412 to 418 in 748710c

	const passedArgs: Args = combinedParameters.args;
	const defaultArgs: Args = Object.entries(
	argTypes as Record<string, { defaultValue: any }>
	).reduce((acc, [arg, { defaultValue }]) => {
	if (defaultValue) acc[arg] = defaultValue;
	return acc;
	}, {} as Args);

• This issue seems to be fixed if we check for argType before progressing. This seems like it could be appropriate if the whole point of this code is to set a default value for args?
• I think the if (defaultValue) check should be more precise, and not conflate all false-y values.

const passedArgs: Args = combinedParameters.args;
const defaultArgs: Args = Object.entries(
-    argTypes as Record<string, { defaultValue: any }>
-).reduce((acc, [arg, { defaultValue }]) => {
-    if (defaultValue) acc[arg] = defaultValue;
+    argTypes as Record<string, { defaultValue: any } | undefined>
+).reduce((acc, [arg, argType]) => {
+    if (argType && typeof argType.defaultValue !== 'undefined') acc[arg] = defaultValue;
    return acc;
}, {} as Args);

I haven’t checked why the error is silently swallowed and doesn’t just break the tests.

---

Minimal repro if it helps – CRA + default Storybook setup + Storyshots, and a story with null in the Button stories: https://github.com/thibaudcolas/storyshots-null-args

[shilman]

Thanks so much @thibaudcolas -- I'll try out your fix and see what it does. I'm very interested in fixing things on the args side of things, don't have the bandwidth to fix the storyshots side, so PRs welcome on better error handling there.

[Hypnosphi]

Related: #12729

[ChristianIvicevic]

I was bashing my head against the desk when I found this issue after four hours of experimenting. It'd be awesome if this gets fixed soon as it has been reported in September. The changes in #12809 do partially fix the execution of tests via storyshots and I am currently monkey-patching it after installing the dependency. I had to check for undefined and null.

Here is the patch based on 6.1.0-beta.0 I am using with patch-package for the time being:

diff --git a/node_modules/@storybook/client-api/dist/story_store.js b/node_modules/@storybook/client-api/dist/story_store.js
index 097134a..9ff3fba 100644
--- a/node_modules/@storybook/client-api/dist/story_store.js
+++ b/node_modules/@storybook/client-api/dist/story_store.js
@@ -404,10 +404,9 @@ var StoryStore = /*#__PURE__*/function () {
           globalTypes = _this$_globalMetadata3 === void 0 ? {} : _this$_globalMetadata3;
       var defaultGlobals = Object.entries(globalTypes).reduce(function (acc, _ref8) {
         var _ref9 = _slicedToArray(_ref8, 2),
-            arg = _ref9[0],
-            defaultValue = _ref9[1].defaultValue;
+            arg = _ref9[0];
 
-        if (defaultValue) acc[arg] = defaultValue;
+        if (_ref9[1] && typeof _ref9[1].defaultValue !== 'undefined' && _ref9[1].defaultValue !== null) acc[arg] = _ref9[1].defaultValue;
         return acc;
       }, {});
       var allowedGlobals = new Set([].concat(_toConsumableArray(Object.keys(initialGlobals)), _toConsumableArray(Object.keys(globalTypes)))); // To deal with HMR & persistence, we consider the previous value of global args, and:
@@ -729,10 +728,9 @@ var StoryStore = /*#__PURE__*/function () {
       var passedArgs = Object.assign({}, this._kinds[kind].parameters.args, storyParameters.args);
       var defaultArgs = Object.entries(argTypes).reduce(function (acc, _ref17) {
         var _ref18 = _slicedToArray(_ref17, 2),
-            arg = _ref18[0],
-            defaultValue = _ref18[1].defaultValue;
+            arg = _ref18[0];
 
-        if (defaultValue) acc[arg] = defaultValue;
+        if (_ref18[1] && typeof _ref18[1].defaultValue !== 'undefined' &&  _ref18[1].defaultValue !== null) acc[arg] = _ref18[1].defaultValue;
         return acc;
       }, {});
       var initialArgs = Object.assign({}, defaultArgs, passedArgs);

And this is the patch file for version 6.0.28:

diff --git a/node_modules/@storybook/client-api/dist/story_store.js b/node_modules/@storybook/client-api/dist/story_store.js
index 9b0a8ba..6c8cf7c 100644
--- a/node_modules/@storybook/client-api/dist/story_store.js
+++ b/node_modules/@storybook/client-api/dist/story_store.js
@@ -383,10 +383,9 @@ var StoryStore = /*#__PURE__*/function () {
           globalTypes = _this$_globalMetadata3 === void 0 ? {} : _this$_globalMetadata3;
       var defaultGlobals = Object.entries(globalTypes).reduce(function (acc, _ref8) {
         var _ref9 = _slicedToArray(_ref8, 2),
-            arg = _ref9[0],
-            defaultValue = _ref9[1].defaultValue;
+            arg = _ref9[0];
 
-        if (defaultValue) acc[arg] = defaultValue;
+        if (_ref9[1] && typeof _ref9[1].defaultValue !== 'undefined' && _ref9[1].defaultValue !== null) acc[arg] = _ref9[1].defaultValue;
         return acc;
       }, {});
       var allowedGlobals = new Set([].concat(_toConsumableArray(Object.keys(initialGlobals)), _toConsumableArray(Object.keys(globalTypes)))); // To deal with HMR & persistence, we consider the previous value of global args, and:
@@ -640,10 +639,9 @@ var StoryStore = /*#__PURE__*/function () {
       var passedArgs = combinedParameters.args;
       var defaultArgs = Object.entries(argTypes).reduce(function (acc, _ref16) {
         var _ref17 = _slicedToArray(_ref16, 2),
-            arg = _ref17[0],
-            defaultValue = _ref17[1].defaultValue;
+            arg = _ref17[0];
 
-        if (defaultValue) acc[arg] = defaultValue;
+        if (_ref17[1] && typeof _ref17[1].defaultValue !== 'undefined' && _ref17[1].defaultValue !== null) acc[arg] = _ref17[1].defaultValue;
         return acc;
       }, {});
       var initialArgs = Object.assign(Object.assign({}, defaultArgs), passedArgs);

[thibaudcolas]

@ChristianIvicevic it’s not clear to me why you checked for null as well? Your suggested patch makes it impossible to use null as the default value, which is perfectly valid (at least for React projects), and not the same as undefined. (typeof undefined vs typeof null).

[ChristianIvicevic]

@thibaudcolas Let me preface this by saying I don't know the codebase of Storybook at all so this was merely trial and error for me and hence I don't know any relations and intricacies here.

I noticed before checking for null that my stories would have random and inexplicable runtime errors when having props defined either implicitly (by not defining a value for optional props) or explicitly as undefined. In fact some props were suddenly null even though I explicitly set them in the story causing errors where calls to methods such as toLocaleString failed since they operated on a null value even though it was supplied. My assumption is that the props got mixed up.

Furthermore due to my personal code style I never work with null at all so I can't say how props behave that you want to allow to be null in the first place.

[ChristianIvicevic]

I just tested the latest beta of v6.1 and my patch is no longer necessary. Storyshots have been fixed for all occurences of undefined and null in my project.

[shilman]

Hooray! Closing.