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

feat: add preHandlers or transforms for AST data for react functionComponents in typescript #85

Closed
wants to merge 2 commits into from

Conversation

Winner95
Copy link

Hello,

I would like to suggest a PR with preHandlers in order get defined type from React.functionComponent definitions working in Storybook without rewriting these definitions for the users of storybook. Please find example here.

I made react.functionComponent definitions written in typescript work with react-docgen via this custom handler. It helps to get defined type from React.functionComponent definition:

type ComponentType = {
    text?: string;
};

const Component: React.FunctionComponent<ComponentType> = (props) => (...);

In order to make it work we need to add preHandlers - which can be called included in handlers array and called first before the actual default handlers.

Main benefit of this approach - it allows to support and cover UI-component props in storybook without rewriting definitions of types for props. Initial discussion was raised in react-docgen repo.

@danielduan
Copy link
Member

@Winner95 Hi thanks for the PR. I'm not sure if I understand the problem this PR is trying to solve. Please correct me if I'm wrong.

It sounds like there is a bug regarding how react-docgen parses TypeScript in a specific case. The fix you created gets around the existing bug through a custom handler. This custom handler must be added before react-docgen in order to bypass the bug.

From what I understand, this use case seems a little narrow and I'm not sure that we need yet another way of passing custom handlers in. What other ways would a user need preHandlers that would not work with the existing customHandlers?

@Winner95
Copy link
Author

Hi @danielduan ,

thank you for your time! Yes, that is correct. I believe that this might be the only case in current React component structure for now. There might be other more complex cases, but they will be probably even more rare - such as passing prop definition to components stories.

I also agree that it might seem to be narrow case, while functional components are becomming popular concept in React written in Typescript.

The issue that I was thinking to solve was to add support for props identification for users, which already have large codebase with components written in certain style:

const Component: React.FunctionComponent<Props> = (props) => {...}

instead of rewriting it to:

const Component: React.FunctionComponent = (props: Props) => {...}

or

const Component = (props: Props) => {...}

Storybook can provide props support from the box with babel-plugin-react-docgen with usage of customPreHandler. It will allow users of storybook not to rewrite codebase props definition, but solve the problem by adding only several lines in storybook config.

Do you think there is no need in this idea for now?

@koop
Copy link

koop commented Jun 27, 2020

Fwiw, I'm currently encountering a related problem that wouldn't be addressed by this patch, but would be addressed by allowing custom resolvers (#82):

const Example: React.AbstractComponent<Props> = createComponent();

The issue is that createComponent() isn't detected by any of the default resolvers, so the line can't be processed by any handlers. Instead, I use a custom resolver to create a temporary AST that looks like function Example(props: Props) {} which can then be parsed by the default handlers. The same approach could work here as well.

@jimmyandrade jimmyandrade changed the title add preHandlers or transforms for AST data for react functionComponents in typescript feat: add preHandlers or transforms for AST data for react functionComponents in typescript Sep 30, 2020
Copy link
Member

@jimmyandrade jimmyandrade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants