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

Next/refactor #51

Closed
wants to merge 12 commits into from
Closed

Conversation

binomialstew
Copy link

There is a lot of redundancy between this plugin and react-docgen. This PR relies on react-docgen as much as possible to eliminate this redundancy. For example, we don't need to check the AST visitor types because react-docgen handles the doc generation based on the existing components. This PR also adds support for multiple component docs export from the same file. It also addresses an issue where the generated docs would be stored under an incorrect name (the last component defined in the file) even when they were defined on another component.

@danielduan
Copy link
Member

@CLL80 thanks so much for this PR and converting over to using the Recast AST walker.

Let me dig through this PR a bit more and run some tests on my Storybook instances.

@binomialstew
Copy link
Author

No problem. Let me know if I can clarify anything.

Recast is used to get the actual name of the component where props are defined, and only because the actualNameHandler is heavily based on react-docgen's default displayNameHandler—a dependency of this plugin which already uses recast. I'm sure we could do this without it but am not sure this is necessary.

This is to correct an error that you may have seen and I know has caused some pain among the users of this plugin—docs may be output under an incorrect name if there is more than one component export—it would take the name of the last component in the file—whether or not the docs actually pertained to this.

@danielduan danielduan changed the base branch from next/2.0 to master June 17, 2018 15:12
@danielduan
Copy link
Member

danielduan commented Jun 17, 2018

@binomialstew can you do me a huge favor and rebase your branch? i just rebased and merged my other stuff. thanks!

edit: actually, don't worry about this. I brought over your changes to #54 and I'm adding a few more tests from this and the storybook issue tracker.

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

Successfully merging this pull request may close these issues.

3 participants