storybookjs / storybook

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

addon withInfo propTablesExclude does not exclude #3758

Closed VladimirPittner closed 5 years ago

VladimirPittner commented 6 years ago

Bug or support request summary

I am trying to exclude components from being shown in Prop Tables section like mentioned herer: docs https://github.com/storybooks/storybook/tree/master/addons/info#options-and-defaults

{
/**
   * Exclude Components from being shown in Prop Tables section
   * Accepts an array of component classes or functions
   * @default []
   */
  propTablesExclude: Array<React.ComponentType>,
}

However it does not seem to work and I am getting Prop Tables even for excluded components.

Steps to reproduce

Here is small sample code to test propTablesExclude:

// @flow
import React from 'react';
import { storiesOf } from '@storybook/react';
import { withInfo } from '@storybook/addon-info';

/* eslint-disable react/no-unused-prop-types,react/require-default-props */
type PropsType = {
  label: string,
  onClick?: Function,
  disabled?: boolean,
};

const Button = ({ label, onClick, disabled }: PropsType) => (
  <button onClick={onClick} disabled={disabled}>
    {label}
  </button>
);

Button.defaultProps = {
  disabled: false,
  onClick: () => {},
};

storiesOf('Test', module).add(
  'Exclude component from prop tables',
  withInfo({
    text: 'This can exclude extraneous components from being displayed in prop tables.',
    propTablesExclude: [Button],
  })(() => (
    <div>
      <Button label="Button" />
    </div>
  ))
);

Here is screenshot of Prop Tables for Button being not excluded: screenshot

Please specify which version of Storybook and optionally any affected addons that you're running

    "@storybook/addon-info": "4.0.0-alpha.9",
    "@storybook/react": "4.0.0-alpha.9",

Where to start

My humble guess is that something goes wrong somewhere here: https://github.com/storybooks/storybook/blob/master/addons/info/src/components/Story.js#L314-L324

if (
  typeof children === 'string' ||
  typeof children.type === 'string' ||
  (Array.isArray(this.props.propTablesExclude) && // also ignore excluded types
    ~this.props.propTablesExclude.indexOf(children.type)) // eslint-disable-line no-bitwise
) {
  return;
}
if (children.type && !types.has(children.type)) {
  types.set(children.type, true);
}

When I have changed this code a bit into something like this:

if (
  typeof children === 'string' ||
  typeof children.type === 'string' ||
  ~this.props.propTablesExclude.toString().indexOf(children.type) // eslint-disable-line no-bitwise
) {
  return;
} else if (!types.has(children.type)) {
  types.set(children.type, true);
}

it seemed to fix the issue. But I am not sure if this is the correct/safe way. It would be cool if someone could investigate this.

Thanks

Keraito commented 6 years ago

Hey @VladimirPittner, that doesn't sound right, especially because your code does seem alright. Could you try moving all the stuff from the component you're using into a separate file, import it into your stories file and see if it still fails?

Also, could you elaborate a bit on your proposed changes, at first glance it's hard to spot what you exactly changed and why 😃

VladimirPittner commented 6 years ago

Hey @Keraito, thanks for your reply. I am not exactly sure what do you mean but "moving all the stuff from the component to separate file", but if you mean that instead of defining the component Button in the same file as the story, to use it in this way?:

// @flow
import React from 'react';
import { storiesOf } from '@storybook/react';
import { withInfo } from '@storybook/addon-info';
import { Button } from './Button';

storiesOf('Test', module).add(
  'Exclude component from prop tables',
  withInfo({
    text: 'This can exclude extraneous components from being displayed in prop tables.',
    propTablesExclude: [Button],
  })(() => (
    <div>
      <Button label="Button" />
    </div>
  ))
);

then that is how I actually have things in 'real life' app. I tested both approaches and the result is same.

To answer the second question: When I was trying to debug what is going on, I have found that this line of code:

this.props.propTablesExclude.indexOf(children.type)

always returns -1 even if the children.type (supposed to be function) and the entry in propTableExclude (supposed to be function) were "the same" (I guess this is the part where I am missing something about object/functions/arrays/whatever in javascript). So I tried to play with it a little and kinda "hacked" it by transforming the propTableExclude to string in which then children.type can be found and therefore the condition is met and we hit the return.

Additionally I have removed:

(Array.isArray(this.props.propTablesExclude) &&

from the condition of first if statement. (cause I have not seen a reason for it anymore) And I have merged those two if statements to one if {} else if {}

As I said, this is most likely not a proper way to fix it, but I hope it will at least help to understand the problem a bit.

Thanks V

EDIT: argh now I see there was typo in the original post code example _this4 ... what a nonsense 🤦‍♂️ ... it should be just this ... ~gonna fix it~ fixed

Keraito commented 6 years ago

but if you mean that instead of defining the component Button in the same file as the story, to use it in this way?

Yes this was what I meant, sorry that it wasn't entirely clear.

I tested both approaches and the result is same.

Hmm that's very weird 😕

even if the children.type (supposed to be function) and the entry in propTableExclude (supposed to be function) were "the same"

Just out of curiosity since you said you played around with it, did you debug that both values were the same, or are you assuming this based on your code?

I actually tried copy pasted your code into the example stories in the Storybook codebase, and it works like expected, without the Proptypes for Button 😕 Could you also provide me your .storybook/config.js?

GrtDev commented 6 years ago

I seem to be running into the same issue. I'm using TypeScript that is transpiled through babel. I suspect the reference is somehow lost in translation.. :(

Keraito commented 6 years ago

@GrtDev could you provide more detailed information, like version numbers and specific implementation, on your scenario (in a new issue)?

VladimirPittner commented 6 years ago

Hi @Keraito, unfortunately I cannot provide you the config. It is true however it is fairly complex and there might be something strange going there too. And we use flow, that is also another thing I have seen few times mentioned in bugs here :/.

Just out of curiosity since you said you played around with it, did you debug that both values were the same, or are you assuming this based on your code?

Its based on debug and now when I did it second time I realized that however both values are functions they are slightly different.

Here is screenshot of dummy debug: screen CHILDREN represents children.type EXCLUDE represents first item of this.props.propTablesExclude INDEXOF is this.props.propTablesExclude.indexOf(children.type) INDEXOF FIXED is this.props.propTablesExclude.toString().indexOf(children.type)

Not sure if if this brings more clarity or confusion here 😄, and I guess I am missing something clearly obvious here 🤔.

Anyway you said you tested it in Storybook codebase and it worked so its possible its something on our end.

Thanks for your help

GrtDev commented 6 years ago

@Keraito My setup is fairly complex as its tied together using multiple packages in a mono-repo. So providing the details is not so straight forward.

The project is entirely written in TypeScript and transpiled through webpack / babel-loader using this config:

presets: [
  [
    require.resolve('@babel/preset-env'),
    {
      targets: {
        browsers: ['last 2 versions'],
      },
      modules: false,
    },
  ],
  require.resolve('@babel/preset-stage-3'),
  require.resolve('@babel/preset-react'),
  require.resolve('@babel/preset-typescript'),
],
plugins: [
  require.resolve('react-hot-loader/babel'),
  require.resolve('loadable-components/babel'),
],

Also the react-docgen-typescript-loader is added for storybook before the babel-loader:

loader.use.push({
  loader: require.resolve('react-docgen-typescript-loader'),
  options: {
    tsconfigPath: paths.tsConfig,
    // Skip props without documentation as this is the only way to filter out all the default HTML props from components
    skipPropsWithoutDoc: true,
    skipPropsWithName: ariaProps,
  },
})

Not sure if this information is of any help... When I can I'll try to setup a small test project to reproduce the bug.

Keraito commented 6 years ago

When I can I'll try to setup a small test project to reproduce the bug.

This would be very helpful!

GrtDev commented 6 years ago

@Keraito Ok I quickly tossed this together. On a side-note: next to ignoring certain components when its used as a HOC, it would also be really nice to configure this globally. This would make sense for components that you only use as placeholders or wrappers in your styleguide. Let me know if this example works for you. Thanks in advance for your help!

Keraito commented 6 years ago

Thanks @GrtDev for the reproduction.

On a side-note: next to ignoring certain components when its used as a HOC, it would also be really nice to configure this globally.

Regarding this, it is actually possible to use any of the (HOC) options as a global option aswell. See this part of the documentation for the 3.4 release and this part for the alpha release. Does that help instead?

GrtDev commented 6 years ago

Thanks for the response @Keraito. I see, that looks good! I misread this comment by danielduan earlier and thought setting it globally would not be possible. But he was talking about mixing local and global settings. But this seemed to have been made available then in the alpha release?

Still though, I have not been able to get this specific option (propTablesExclude) working with TypeScript.

typosadovsky commented 5 years ago

Can we reopen this issue? It doesn't look like it's been solved. I am using JSS in my React app and the option (propTablesExclude) does not exclude the JSS component propTable (which is wrapped in an HOC)

tomegz commented 5 years ago

For me propTablesExclude doesn't work (other parameters work correctly)

nimaiwalsh commented 5 years ago

As above, same issue. propTablesExclude does not work. Also using TypeScript.

tomhuze commented 5 years ago

Still not working as of v5.0.5.

crimx commented 5 years ago

This is indeed a very confusing issue. Not likely to get fixed. How about additional options using other properties to identify components? Maybe displayName?

crimx commented 5 years ago

This bug is caused by react hot loader and I've fired a PR two weeks ago. Somehow being ignored? Am I missing something?

shilman commented 5 years ago

@crimx Sorry your PR hasn't been addressed yet. There are about 40 other PRs active, we're trying to get 5.2 shipped, and addon-info doesn't really have an owner right now. Hope you can be patient on this one.

crimx commented 5 years ago

I see.

shilman commented 5 years ago

Ermahgerd!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.2.0-beta.41 containing PR #7409 that references this issue. Upgrade today to try it out!

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

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

kvedantmahajan commented 4 years ago

8777 is related to Typescript setup and propTables not working. I'm just holding the draft of my release email because of this @shilman. Certainly not a good thing to know towards the end of development. I did have a look at options and assumed it will work for me if I need it. Now, that's not working. Is there a workaround for this at least? FYI, I'm using 5.2.3 and it's not working.