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

Template.bind on imported variable results in variable name as string literal rather than variable value #12635

Closed
Multihuntr opened this issue Oct 1, 2020 · 20 comments

Comments

@Multihuntr
Copy link

Describe the bug
When using the Template.bind({}) method of creating stories, if a default value of the component is a variable which resolves to a value which is imported, then the value which is passed is a string literal of the variable name, rather than the variable value.

This only appears to happen when you use Template.bind({}).

To Reproduce

Here is a MWE.

Use create-react-app. Install storybook. Create these files:

Component.js

import React from 'react';

import { VAL } from './someconsts';
const localVal = 'world';
export const Component = ({ greet, other }) => <div>{greet}, {other}</div>;
Component.defaultProps = {
  greet: VAL,
  other: localVal
};

someconsts.js

export const VAL = 'Hello';
export const SAL = 'Salutations';

Component.stories.jsx

import React from 'react';

import { SAL } from './someconsts';
import { Component } from './Component';

export default {
  title: 'Component',
  component: Component,
};

const Template = (args) => <Component {...args} />;


export const DefaultBind = Template.bind({});
DefaultBind.args = {};
export const DefaultClosure = () => <Component />;


export const ExileBind = Template.bind({});
ExileBind.args = { greet: SAL, other: 'exile' };
export const ExileClosure = () => <Component greet={SAL} other={'exile'} />;

The rendered text in each case is:

  • DefaultBind: VAL, world
  • DefaultClosure: Hello, world
  • ExileBind: Salutations, exile
  • ExileClosure: Salutations, exile

Additionally, in the Storybook web interface, it shows this for both closure stories:
image

Expected behavior

  1. It would be able to import variable values from a different file
  2. It would not silently replace the value with a string literal of the variable name.

Screenshots
See above

Code snippets
See above

System

Environment Info:

  System:
    OS: Linux 4.15 Alpine Linux
    CPU: (8) x64 Intel(R) Core(TM) i7-4790S CPU @ 3.20GHz
  Binaries:
    Node: 12.18.3 - /usr/bin/node
    Yarn: 1.22.4 - /usr/local/bin/yarn
    npm: 6.14.7 - /usr/bin/npm
  npmPackages:
    @storybook/addon-actions: ^6.0.12 => 6.0.12 
    @storybook/addon-essentials: ^6.0.12 => 6.0.12 
    @storybook/addon-links: ^6.0.12 => 6.0.12 
    @storybook/node-logger: ^6.0.12 => 6.0.12 
    @storybook/preset-create-react-app: ^3.1.4 => 3.1.4 
    @storybook/react: ^6.0.12 => 6.0.12 

Additional context
I am running this in a Docker container based on mhart/alpine-node:12, where the only difference is that I map my user from the host machine as the default user inside the container.

Is this related to #12455 ? I initially thought it was, but now I'm not so sure.

@goranjviv
Copy link

goranjviv commented Oct 1, 2020

It doesn't have to be a complex object, the prop just needs to reference an object.
orientation: "whatever string" + Orientations.HORIZONTAL + 'jfdkslfj'
And orientation ends up being that entire expression.
I'm not sure if the object needs to be imported or not 🤔 I'm not sure if I used some locally defined objects when debugging this.

@Multihuntr
Copy link
Author

Multihuntr commented Oct 2, 2020

I changed Component.js to:

import React from 'react';

import { A } from './someconsts';
const B = 'B'
const C = 'C' + 'c';
const D = B + 'D';
const E = { inner: 'e' };
const F = parseInt('123');
const G = 'G' + parseInt('123');
const onetwothree = '123';
const J = 'J' + parseInt(onetwothree);
export const Component = ({ a,b,c,d,e,f,g,h,i,j }) =>
  <div>{a} | {b} | {c} | {d} | {e} | {f} | {g} | {h} | {i} | {j}</div>;
Component.defaultProps = {
  a: A,
  b: B,
  c: C,
  d: D,
  e: E.inner,
  f: F,
  g: G,
  h: 'H' + 'h',
  i: B + 'i',
  j: J,
};

(where A === 'Hello') And Component.stories.jsx to

import React from 'react';

import { Component } from './Component';

export default {
  title: 'Component',
  component: Component,
};

const Template = (args) => <Component {...args} />;


export const DefaultBind = Template.bind({});
DefaultBind.args = {};
export const DefaultClosure = () => <Component />;

And this is what gets rendered in each of the stories now:

DefaultBind:    A | B | Cc | B + 'D' | e | 123 | G123 | Hh | B + 'i' | 'J' + parseInt(onetwothree)
DefaultClosure: Hello | B | Cc | BD | e | 123 | G123 | Hh | Bi | J123

Thus, it does not appear to have any problems with an object, but, rather, any computation involving variables. Interestingly, computation involving expressions without variables are executed without issue, even parsing an int. So there is javascript being executed.

@shilman
Copy link
Member

shilman commented Oct 2, 2020

@Multihuntr that's a pretty intense example 🙈

Here's what's going on:

  1. The component/story is processed with react-docgen-typescript (RDT). Storybook treats that as a black box. It produces a string value using some logic that I don't fully grok. Some of these tools do a static analysis of the source code, I think RDT also uses the output of the Typescript compiler, which is also a static analysis but a very sophisticated one.
  2. Storybook evals the string to get actual values. Since the string can contain undefined variables, it's wrapped in a try/catch, and when the eval fails it uses the raw string.

So I think Storybook is doing the eval, succeeding in some cases and producing the actual value, and failing in some cases and falling back to the string produced by static analysis.

@jonniebigodes I'm not sure how to document this. I don't know how much better we can do than what we have now

@jonniebigodes
Copy link
Contributor

@Multihuntr sorry you ran into the issue, but glad that you supplied us with some context on this. @shilman going over this i have to agree with you and keep the documentation as is as to avoid introducing a bit of entropy.

@Multihuntr
Copy link
Author

Multihuntr commented Oct 5, 2020

If storybook just evals something from react-docgen-typescript (RDT), then it's RDT which is not following the imports with it's static analysis. So;

  • Is this expected behaviour by RDT? (particularly test case D seems like it should be able to work)
  • Is there anything to be done around it? (e.g. importing the file within storybook, not just using RDT, to use javascript to calculate the context before running eval)

@shilman
Copy link
Member

shilman commented Oct 5, 2020

@Multihuntr you can always override the output of autogeneration if it's not producing what you want:

https://github.com/storybookjs/storybook/blob/next/addons/controls/README.md#my-controls-arent-being-auto-generated-what-should-i-do

AFAIK RDT is the best thing out there right now for generating this kind of info.

@Multihuntr
Copy link
Author

Thanks. Although, the controls aren't that important to me. I only use Storybook as a way of rendering test cases in a browser. I was surprised to get a string literal, since I didn't know that RDT was being used.

I don't need any help with anything; I'm just going to use DefaultBind.args = Component.defaultProps.

I don't use all the features of Storybook, but I'm not clear what is being autogenerated from RDT that can't be read after-the-fact, once the component is loaded.

@shilman
Copy link
Member

shilman commented Oct 5, 2020

Storybook consumes the following information about component props from RDT:

  • name
  • type
  • default value
  • description

@Hypnosphi
Copy link
Member

DefaultBind.args = Component.defaultProps

@shilman shouldn't we do it by default?

@shilman
Copy link
Member

shilman commented Oct 8, 2020

@Hypnosphi what about typescript

@Hypnosphi
Copy link
Member

@shilman I though you can have defaultProps in TS component, am I missing something?

@shilman
Copy link
Member

shilman commented Oct 8, 2020

I thought typically people specify the default values by:

const Foo: FC<FooProps> = ({ bar='bar', baz=2 }) => <>whatever</>

I guess we can spread in ...defaultProps even if it's not specified.

@Hypnosphi
Copy link
Member

It's a common pattern in JS as well. Does react-docgen recognize it though?

@shilman shilman self-assigned this Oct 8, 2020
@zoily
Copy link

zoily commented Oct 8, 2020

Hello ! I have the same issue.
Used constant :
const buttonColor = {
PRIMARY: 'primary'
}
When I import this constant object in my component the control set the value to "buttonColor.PRIMARY", if I declare it in the file, the value is "primary". I had same result with destructuring and Component.defaultProp to set values.
Generated doc failed :
image

I use ES6 with flow (for static schema) and functional components.

@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 8, 2020

DefaultBind.args = Component.defaultProps

Unfortunately, it breaks automatic actions configured with {argTypesRegex: '^on.*'} if Component.defaultProps contains values for corresponding props =( Let's make sure it doesn't

@Hypnosphi
Copy link
Member

Storybook evals the string to get actual values. Since the string can contain undefined variables, it's wrapped in a try/catch, and when the eval fails it uses the raw string.

@shilman Maybe we should just resolve to undefined in this case? This will work both with defaultProps and default argument values

@zoily
Copy link

zoily commented Oct 13, 2020

I followed your solution, and here is what I did as a quick fix :

const Template = (args) => <Button {...args} />
export const Primary = Template.bind({})
Primary.args = {
  ...Button.defaultProps,
  children: 'Primary'
}

Not really sexy cause it forced me to take default values out of destructuring props in my functional component, and to add a line on each story. Is there a better way ?

PS: In response to last Hypnosphi post, I am not sure about what I am talking about, but why Storybook would fall into a catch if I have a value in my default value constant :

//constant.js
export const color = {
  PRIMARY: 'primary'
}
//comp.js
export const comp({color: color.red}) {...}

color.red should not be undefined there

Update: Okay I think I understood, the issue comes from the bind({}) function and not from the import

@alfnielsen
Copy link

This is a serious problem.
What I can understand from this conversation,
StoryBooks are using the default values from the docgen which make no sense ?
It should only be used for the description, not the actual story code.

And from our tests its not the .bind, (At least not the Templte.bind({})we write in the story)
it comes from a complex data type in the native components default values:
(So it does seem like its Storybook that somewhat uses the docgen result... )

export interface ISelectableListProp<T = string> {
  rowRender?: (
    item: T,
    selected: boolean,
    onClick: (item: T) => void
  ) => ReactElement
  (...)
}

const SelectableList = <T extends any = string>({
  rowRender = (item, selected, onClick) => (
    <StandardStringListItem
      selected={selected}
      onClick={() => onClick?.(item)}
      value={`${item}`}
    />
  ),
 (...)

The Hack we are using, forces the default value to be undefined in the stories that dont defines a value:

const Template = <T extends {}>(): Story<ISelectableListProp<T>> => args => {
  // HACK: Storybook (v6.0.26) can't correctly handle default-prop values that returns virtual dom.
  // force undefined in the actual story element + add "disable: true" for its control.
  console.log(args.rowRender) // <- This is a string (the functions toString / maybe from docgen...)
  args.rowRender = undefined
  return <SelectableList<T> {...args} />
}

@shilman shilman modified the milestones: 6.1 docs, 6.1 args Oct 22, 2020
@shilman shilman modified the milestones: 6.1 args, 6.2 args Nov 24, 2020
@stale
Copy link

stale bot commented Dec 25, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@shilman
Copy link
Member

shilman commented Feb 20, 2021

Shiver me timbers!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.2.0-alpha.30 containing PR #13919 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 Feb 20, 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

8 participants