strothj / react-docgen-typescript-loader

Webpack loader to generate docgen information from Typescript React components.
Other
360 stars 47 forks source link

Suggestion: consider removal of `includes` and `excludes` options #15

Closed amacleay closed 5 years ago

amacleay commented 6 years ago

Reason: duplicative with Webpack's Rule.exclude and Rule.include options.

I bring this up because I am using a non-standard Storybook webpack configuration: I want react-docgen-typescript to run on a particular node_modules folder. It took me quite a lot of debugging to figure out that, even though I was invoking invoking this loader for my files, I was not running the loader on these files because they matched the default excludes.

@storybook/react@3.4.7, @storybook/core@3.4.7, react-docgen-typescript-webpack-plugin@1.1.0, react-docgen-typescript@1.6.0

Sample webpack configuration:

// cat webpack.config.js
module.exports = (baseConfig, env, config) => {
  config.module.rules.push({
    test: /\.(ts|tsx)$/,
    // Exclude all node_modules EXCEPT @corp/components
    // so that the plugins pick up the source code from that lib
    exclude: /\/node_modules\/(?!@corp\/components)/,
    use: [
      require.resolve('awesome-typescript-loader'),
      require.resolve('react-docgen-typescript-loader'),
    ],
  });
  return config;
};

Fixed version w/ hacky excludes option:

// cat webpack.config.js
module.exports = (baseConfig, env, config) => {
  config.module.rules.push({
    test: /\.(ts|tsx)$/,
    // Exclude all node_modules EXCEPT @corp/components
    // so that the plugins pick up the source code from that lib
    exclude: /\/node_modules\/(?!@corp\/components)/,
    use: [
      require.resolve('awesome-typescript-loader'),
      {
        loader: require.resolve('react-docgen-typescript-loader'),
        options: {
          excludes: [],
        },
      },
    ],
  });
  return config;
};

(Incidentally, I believe that the way the plugin acts with an includes parameter but without an excludes parameter is confusing: if I use options: { includes: [new RegExp('node_modules/some_module')] } the loader will not pick up node_modules/some_module/some_file.tsx; to make it work I would need to additionally provide an empty excludes, as in options: { includes: [new RegExp(node_modules/some_module'), excludes: [] })

strothj commented 6 years ago

You're using a monorepo setup correct? I think I ended up with a similar situation when doing something similar. I'll think this over in more detail on Sunday and see what direction I'd like to take here.

strothj commented 6 years ago

I'm looking into this.

The original concern for me was that I was using the babel-typescript-preset. The same loader is used for both Javascript and TypeScript compilation. I was also hoping to make the startup process of Storybook less CPU intensive on large projects by limiting the docgen to only Storybook stories.

I don't think this choice has made a measurable difference, the performance is still awful.

amacleay commented 6 years ago

Yes, I'm essentially using a monorepo setup.

(In fact, it's not actually currently set up as a monorepo, but it is an obvious candidate: more than one repo of component libraries, but one storybook repo to showcase together)

Thanks for taking a look!

strothj commented 6 years ago

@amacleay

Hello,

I've released a release candidate with this change.

You can review the change log here: https://github.com/strothj/react-docgen-typescript-loader/blob/v3/README.md

The package is available as react-docgen-typescript-loader@next.

I'd like to release v3 this coming Wednesday. If you have any problems or feedback, it would be helpful.

amacleay commented 6 years ago

I tested with your new code and it does seem to do what's on the tin. Thanks!

While testing, though, I ran into another issue which requires a bit of work. I'll add that as another issue shortly. Thanks for all your work.

nickserv commented 5 years ago

This should be closed by 3.0.0, right?

strothj commented 5 years ago

This should be closed by 3.0.0, right?

Yes, this was taken care of as v3. Thanks.