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

Allow more configuration of react-docgen #88

Merged
merged 3 commits into from
Sep 23, 2020
Merged

Conversation

phated
Copy link
Contributor

@phated phated commented Sep 22, 2020

In preparation for my react-docgen work that will allow importing types across a codebase, I also found that this babel plugin doesn't allow for as much configuration as is required to wire that up.

So, I expanded the options to allow passing more things to react-docgen. I also added more information to the options table to help people that will want to customize this plugin.

This also closes #82 & closes #83

@shilman
Copy link
Member

shilman commented Sep 22, 2020

@phated this looks great, and i appreciate the amount of work that went into getting this whole thing off the ground, esp in your react-docgen PR. you are a hero.

before i merge, can you sketch out how these options are going to be used when it's all hooked up in storybook? will these options be provided by storybook? or provided by users who are configuring storybook? i'd like to understand how it all fits together. thanks! 🙏

@phated
Copy link
Contributor Author

phated commented Sep 22, 2020

Definitely! In an attempt to reduce friction on the react-docgen PR, I chose to avoid breaking changes with the "importer" work. This means that the primary function signature didn't change at all and the default "importer" is actually a no-op.

For the storybook use case, I added a new option called importer that can be used to provide a function that resolves the imports. And my PR also includes a "filesystem importer" that resolves modules, parses their AST and caches them in a Map.

When in use within storybook, it should look like this:

{
    test: reactDocgen === 'react-docgen' ? /\.(mjs|tsx?|jsx?)$/ : /\.(mjs|jsx?)$/,
    plugins: [
      [
        require.resolve('babel-plugin-react-docgen'),
        {
          DOC_GEN_COLLECTION_NAME: 'STORYBOOK_REACT_CLASSES',
          importer: reactDocgen.importer.makeFsImporter()
        },
      ],
    ],
}

I'll be working on that PR once things start to settle/land upstream, and I'd also suggest that we allow additional configuration from the end-user similar to the typescriptOptions that can be currently set. My reasoning for that would be that someone might want to provide their own resolver/importer/cache/etc, and I actually already have a need to customize the importer for some weirdness in the project I'm working on.

All that to say, I'm trying to reduce breaking changes upstream and weave customization throughout without making it a pain to use the default setup!

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

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This is a fantastic change @phated -- really good stuff.

Can you add two test new cases?

  • Custom resolver
  • Custom handler

The code in index.test.js should be a good template for what this would look like. I'm happy to talk it through if you need more detail.

@phated
Copy link
Contributor Author

phated commented Sep 23, 2020

@shilman Since resolvers and handlers are a react-docgen concept, I think it's actually correct that we just pass the options through correctly.

I've added tests for defining custom resolvers with a string (which uses one defined by react-docgen), or a custom function (for which I just used a mock wrapping an imported resolver from react-docgen), and a custom function for a handler. The other tests were already testing a custom handler using a string.

This actually helped catch a bug in this module which caused a crash if trying to specify "findExportedComponentDefinition" as the handler because it returns a single element instead of an array. I fixed that issue and made sure to test it in my added tests!

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks for doing that. The system works! 😂

@shilman shilman merged commit a1cfa0b into storybookjs:master Sep 23, 2020
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.

Cannot pass Babel parser options to react-docgen Cannot define custom resolver
3 participants