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

Addon-controls: Improve select arg serialization #13888

Closed
shilman opened this issue Feb 12, 2021 · 8 comments
Closed

Addon-controls: Improve select arg serialization #13888

shilman opened this issue Feb 12, 2021 · 8 comments

Comments

@shilman
Copy link
Member

shilman commented Feb 12, 2021

As a developer,
I want to use non-serializable objects in controls,
so that I can choose between different {images, react components, etc.}

Currently, all keys and values must be JSON serializable (telejson-serializable, actually), which is a serious restriction when components can take arbitrary blob values / complex functions as their inputs.

If we modify the select arg serialization, we can make use of the existing API to support non-serializable objects.

Consider the following example:

export default {
  component: YourComponent,
  title: 'A complex case with a function',
  // creates specific argTypes with options
  argTypes: {
    propertyA: {
      control: {
        type: 'select',
        options: {
          One: <Foo />,
          Two: <Bar />
          Three: </Baz>
        },
      },
    },
  },
};

Currently this will fail because we try to serialize <Foo/>, <Bar/>, and <Baz/>, which are non-serializable. However, if we updated the behavior to serialize One, Two, and Three, and then did a lookup in the options map before the arg values were passed into the story rendering function, then the user could use any value.

This would be a non-breaking change because the serialization behavior is currently an implementation detail.

Fixing this will also mitigate the security restrictions introduced in URL handling: #13803

┆Issue is synchronized with this Asana task by Unito

@tmeasday
Copy link
Member

Is this related? #12613

@shilman
Copy link
Member Author

shilman commented Feb 21, 2021

Yes, seems like this would also fix #12613

@shilman
Copy link
Member Author

shilman commented Mar 1, 2021

@ghengeveld chatted with @tmeasday about this today. his suggestion (which I think makes sense) is to make this a feature of args. So

argTypes: {
  foo: {
    // typical argType stuff
    control: { type: 'select', options: { key1: 'val1', key2: 'val2' }
    mapping: {
      val1: <SomeComplexValue />,
      val2: () => someFunction,
      ...
    }
  }
}

Then it would be up to the StoryStore to transform the serializable values into the complex values before the story is rendered. WDYT?

@shilman
Copy link
Member Author

shilman commented Mar 4, 2021

Shiver me timbers!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.2.0-beta.8 containing PR #14100 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Mar 4, 2021
@ghengeveld
Copy link
Member

Going to reopen this because we changed our minds. The new proposal is:

argTypes.children = {
  options: ['a', 'b', 'c'], // Must be an array
  mapping: {  // Must be an object
    a: <A />,
    b: <B />,
  }, 
  control: {
    type: 'select', // inferred from options existing
    labels: { // Inferred from `options` if set -- must be an object
      a: 'A is a good choice',
      b: 'João',
    }
  }
}

@ghengeveld ghengeveld reopened this Mar 4, 2021
@acerbastimur
Copy link

I wonder if I will be able to use it like that ?

Screen Shot 2021-03-05 at 00 37 23

@ghengeveld
Copy link
Member

@acerbastimur That looks like you want a control on a nested property. Unfortunately that's not possible. However you can use the newly introduced JSON editor for objects such as theme. It doesn't include a color picker though.

Feel free to open a feature request for controls on nested properties. Your example it a legit use case.

@shilman
Copy link
Member Author

shilman commented Mar 9, 2021

Jeepers creepers!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.2.0-beta.12 containing PR #14169 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants