Storybook: addon-viewport unable to fallback to default options

[kevin940726]

Describe the bug
It's a possible bug or misleading documentation. @storybook/addon-viewport accepts an object with key viewport as option, the default value of the option is as follow.

{
  viewports: MINIMAL_VIEWPORTS,
  defaultViewport: 'responsive',
}

However, the values are only optional when the user doesn't specify the viewport option at all, setting just one of them will lead to bug and crash the page.

// This will lead to run-time error
addParameters({
  viewport: {
    defaultViewport: 'iphonex',
  },
});

To Reproduce

I've created a demo repo to reproduce this issue.

1. git clone
2. yarn && yarn run storybook
3. Head to http://localhost:6006
4. Observe errors by the blank screen or in the console
   > Uncaught TypeError: Cannot convert undefined or null to object

Expected behavior

If it's intended (while I think it's not?), then we should probably document it more precisely so that someone else won't encounter this issue again (see #8569, #5304, and #8641 is a variant of this). It takes me quite some time to locate the bug since neither the doc nor the error message is that useful.

I would expect that it could fallback to the default value mentioned in the documentation.

viewport: {} 
// should fallback to
// {
//   viewports: MINIMAL_VIEWPORTS,
//   defaultViewport: 'responsive',
// }

viewport: { viewports: SOME_VIEWPORTS }
// should fallback to
// {
//   viewports: SOME_VIEWPORTS,
//   defaultViewport: 'responsive',  // (or the first key in viewports?)
// }

viewport: { defaultViewport: 'iphonex' }
// should fallback to
// {
//   viewports: MINIMAL_VIEWPORTS,
//   defaultViewport: 'iphonex',
// }

Code snippets
If applicable, add code samples to help explain your problem.

System:

$ npx -p @storybook/cli@next sb info

Environment Info:

  System:
    OS: macOS 10.15.1
    CPU: (8) x64 Intel(R) Core(TM) i7-8559U CPU @ 2.70GHz
  Binaries:
    Node: 13.2.0 - ~/.nvm/versions/node/v13.2.0/bin/node
    Yarn: 1.19.1 - /usr/local/bin/yarn
    npm: 6.13.1 - ~/.nvm/versions/node/v13.2.0/bin/npm
  Browsers:
    Chrome: 78.0.3904.108
    Firefox: 70.0.1
    Safari: 13.0.3
  npmPackages:
    @storybook/addon-viewport: ^5.2.8 => 5.2.8
    @storybook/html: ^5.2.8 => 5.2.8

It should exits in all types (I've tried react and html) of storybook and it's also happening in next version.

Additional context

I think the bug is happening here

https://github.com/storybookjs/storybook/blob/013b535c78b42f5d2f30764d0a0565e7f798c579/addons/viewport/src/Tool.tsx#L127-L133

where the second argument of useParameter will only be used when PARAM_KEY is not exits in the parameters object.

I can help send a PR to fix this, and I also believe that we should export MINIMAL_VIEWPORTS to the users so that they can tweak the default values.

[shilman]

@kevin940726 Hey Kai Hao! Thanks for this very thorough issue!!!

viewport: {} 
// should fallback to
// {
//   viewports: MINIMAL_VIEWPORTS,
//   defaultViewport: 'responsive',
// }

viewport: { viewports: SOME_VIEWPORTS }
// should fallback to
// {
//   viewports: SOME_VIEWPORTS,
//   defaultViewport: 'responsive',  // (or the first key in viewports?)
// }

viewport: { defaultViewport: 'iphonex' }
// should fallback to
// {
//   viewports: MINIMAL_VIEWPORTS,
//   defaultViewport: 'iphonex',
// }

This makes sense. In the second case, I'd vote for the first key! 👍

Any chance you can put together a PR for this?

[kevin940726]

@shilman Hi Michael~ As promised, I find some time to contribute to storybook lol

Sure, will do!

As a second thought, I wonder how many other addons have this issue too? I haven't looked into them, but if they are also using useParameter to provide default values, we might want to fix useParameter itself instead.

[shilman]

If you have time to look at it, that would be awesome!

However, I'm pretty sure this isn't something that can be solved in a generic way. In your case being "smart" for the user using some heuristics makes a lot of sense. I can imagine other cases where we'd want users to simply override the entire configuration object...

[kevin940726]

@shilman

That's a good point! I'll try fix this issue first and then look into other addons if I got time :)

[kevin940726]

In the second case, I'd vote for the first key!

My bad! As it turns out, it's already defaulting to responsive since it's always in the list. The only missing default is viewports, which should be MINIMAL_VIEWPORTS.

[shilman]

w00t!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-beta.23 containing PR #9137 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.