storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.05k stars 9.24k forks source link

Error in addon-docs prop table when using HOC #9023

Open Zenoo opened 4 years ago

Zenoo commented 4 years ago

Describe the bug When using HOCs, the PropTypes aren't correctly displayed in the addon-docs prop table.
Instead, the following error is displayed:

Cannot read property 'classes' of undefined

To Reproduce Steps to reproduce the behavior:

  1. Create an HOC.
  2. Create a story for this component

Expected behavior The prop table should be filled with the component's proptypes.

Screenshots Error

Code snippets

class Alert extends React.Component { ... }
Alert.propTypes = {
  variant: PropTypes.string,
  dismissible: PropTypes.bool,
  icon: PropTypes.elementType,
  classes: PropTypes.object.isRequired
};
Alert.defaultProps = {
  variant: 'primary',
  dismissible: false,
};

export default withStyles(theme => ({
  alert: props => ({
    backgroundColor: theme.palette[props.variant].main
  }),
  message: {
    display: 'flex',
    alignItems: 'center',
  },
  icon: {
    marginRight: theme.spacing(2),
  }
}))(Alert);

System:

System:
    OS: Windows 10 10.0.18362
    CPU: (8) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
  Binaries:
    Node: 10.16.0 - C:\Program Files\nodejs\node.EXE      
    npm: 6.10.1 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: 44.18362.449.0
shilman commented 4 years ago

Workaround: export the pure component & use that instead

Zenoo commented 4 years ago

It seems to work fine, thanks ! In my example, I only added the export for the base class (export class Alert extends React.Component { ... }) and imported like so in my stories:

import Alert, { Alert as AlertComponent } from '......';
gforrest-bw commented 4 years ago

Is there a plan to address this? I'm working on a component library that themes and re-exports components from Material UI, so I don't have control over all of the components I'm documenting.

stale[bot] commented 4 years ago

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 commented 4 years ago

@gforrest-bw absolutely. hopefully we can address this in january as a high-priority bug.

shilman commented 4 years ago

Repro: https://github.com/storybookjs/storybook/commit/e3ab7571c56bbdf508268c679ebb0afe54037605

gaurav5430 commented 4 years ago

For now I ended up using the same approach as suggested by @Zenoo

exporting both the wrapped component and the unwrapped component and then using the unwrapped component only for the component property.

import Alert, { Alert as AlertComponent } from '......';
export default {
    component: AlertComponent
}
gforrest-bw commented 4 years ago

The HOC we use changes the prop contract, so this isn't the most effective workaround for us.

gaurav5430 commented 4 years ago

The HOC we use changes the prop contract, so this isn't the most effective workaround for us.

what do you mean that the HOC changes the props contract ? @gforrest-bw

gforrest-bw commented 4 years ago

We use TypeScript, so a higher-order component can modify the expected props of the final component by removing or adding props. This is all captured and evaluated by TypeScript. The end user of the library will only see the final, post-hoc (no pun intended) props.

gaurav5430 commented 4 years ago

@gforrest-bw

umm, not sure if i understand, so if the HOC adds/removes some props, wouldn't these props still be documented as optional in whatever is the PropTypes equivalent in typescript, possibly the interface for the component props ?

gforrest-bw commented 4 years ago
function exampleHoc(Component) {
  return ({ foo }) => (
    <Component bar={foo} />
  );
}

Is a trivial (and not very useful) example, but in this case the HOC returns a component with props { foo }, but the wrapped component has { bar }. Documenting the wrapped component would give the wrong idea about the usage of the actual library export.

In real life, we tend to use HOCs that inject a few props into the inner component which aren't present at all on the final outer props, so the documentation would show props which don't 'exist,' in theory.

This isn't a showstopper, just noting that the remedy isn't perfect.

gitnarqt commented 3 years ago

Any update on this issue? It just doesn't make sense to add unnecessary exports to the code just to document it properly 🤷🏻‍♂️

patriklarssson commented 1 year ago

Any news on this issue? The extra export works for the moment, but it would be awesome to have it work without it to minimize bugs with faulty import on components with HOCs