React: Switch react-docgen-typescript-loader to react-docgen-typescript-plugin #11106

hipstersmoothie · 2020-06-09T21:28:23Z

What I did

Typescript docgen for react was super slow!
This pull request switches from react-docgen-typescript-loader to react-docgen-typescript-plugin

Differences:

In a webpack plugin instead of loader
This allows for the typescript docgen to happen all at once and be non-blocking. (as a loader it synchronously executes for each file)
It also allows the plugin to only create the typescript compiler and docgen parser once instead of for every file

In my own project this resulted in a huge speed boost:

Before:

After:

How to test

Is this testable with Jest or Chromatic screenshots? no
Does this need a new example in the kitchen sink apps? no
Does this need an update to the documentation? yes

If your answer is yes to any of these, please make sure to include it in your PR.

…-plugin

shilman

This looks amazing, I can't wait to kick the tires.

This is is a pretty big change so expect some back and forth before we get it merged. First question: what about these:

https://github.com/storybookjs/storybook/blob/next/lib/core/src/server/config/defaults.js#L4-L7

shilman · 2020-06-09T21:38:03Z

cc @mrmckeb

hipstersmoothie · 2020-06-09T21:43:02Z

what about these:

Both of those options will be respected.

https://www.npmjs.com/package/react-docgen-typescript-plugin

The only thing I changed with the options for the plugin are skipPropsWithName skipPropsWithoutDoc, and compilerOptions.

`skipPropsWithName` `skipPropsWithoutDoc`

Instead of creating and extra options we just pass propFilter to react-docgen-typescript. Those two options are handled there.

https://github.com/strothj/react-docgen-typescript-loader

`compilerOptions`

I can add this in if everyone wants it, but it seems way simpler to me to just point at a tsconfig in the repo.

mrmckeb

A few questions @hipstersmoothie, but looks good overall.

I can add this in if everyone wants it, but it seems way simpler to me to just point at a tsconfig in the repo.
You mean you would look for a root config?

mrmckeb · 2020-06-10T06:23:49Z

addons/docs/src/lib/sbtypes/proptypes/convert.ts

@@ -43,8 +43,7 @@ export const convert = (type: PTType): SBType | any => {
    case 'elementType':
    default: {
      if (name?.indexOf('|') > 0) {
-        // react-docgen-typescript-loader doesn't always produce proper
-        // enum types, possibly due to https://github.com/strothj/react-docgen-typescript-loader/issues/81
+        // react-docgen-typescript-plugin doesn't always produce proper


This comment seems to have only been partially removed?

This comment relates to the issue @shilman commented on. Will try to resolve this in the underlying library and remove this comment entirely

I understand, but one line was removed and now it doesn't make sense, right?

// react-docgen-typescript-plugin doesn't always produce proper
// this hack tries to parse out values from the string and should be
// removed when RDTL gets a little smarter about this

mrmckeb · 2020-06-10T06:27:05Z

app/react/src/server/framework-preset-react-docgen.ts

@@ -27,24 +30,13 @@ export function babel(config: TransformOptions, { typescriptOptions }: Storybook

 export function webpackFinal(config: Configuration, { typescriptOptions }: StorybookOptions) {
  const { reactDocgen, reactDocgenTypescriptOptions } = typescriptOptions;
-  if (reactDocgen !== 'react-docgen-typescript') return config;
+
+  if (reactDocgen !== 'react-docgen-typescript') {


Why was this changed?

I personally prefer the new code

I can change it back but one line if statement throw me for a loop. the function above this one also had this style of if statement. Can change back if wanted.

I was more pointing out that this change isn't related to any of the work. I think it's good practice to try to only change things related to the task at hand, unless something is being improved. But if you think the change is meaningful, then I'm OK with that.

mrmckeb · 2020-06-10T06:30:54Z

@shilman when this merges, we can make this change in CRA too.

shilman · 2020-06-10T06:45:44Z

The chromatic snapshots look pretty good. ❤️ @hipstersmoothie please take a look at the "enum"-related snapshot diffs. My enum handling code for RDTL is a hack, and it got "slightly worse" in your version. I'm not concerned about the regressions since it didn't work well in the first place. But I wanted to point it out because if you're going to do more hacking on react-docgen-typescript/(-plugin), that would be a really great area to give some attention to.

hipstersmoothie · 2020-06-10T16:09:43Z

You mean you would look for a root config?

Previously with react-docgen-typescript-loader you could either use the default TS options, provide a path to a tsconfig, or provide complierOptions directly to the loader. I didn't implement directly providing compilerOptions in the plugin but it shouldn't be hard

hipstersmoothie · 2020-06-10T16:15:21Z

@shilman you're talking about how react-docgen-typescript outputs unions like this right?

You'd rather it output to be an enum with a values array?

hipstersmoothie · 2020-06-10T16:52:37Z

@shilman Opened a PR on react-docgen-typescript to add an option that will expand complex unions.

styleguidist/react-docgen-typescript#261

Previously the following example would output

With the new option the package will now expand the above to

hipstersmoothie · 2020-06-10T22:36:56Z

I've added the compilerOptions option now

hipstersmoothie/react-docgen-typescript-plugin#1

shilman · 2020-06-11T00:35:45Z

@hipstersmoothie that was fast! and awesome!! I'll try to kick the tires on this today and hopefully merge.

shilman · 2020-06-11T11:28:43Z

Looking fantastic!! 🔥 Here are the numbers I'm seeing for official-storybook on my machine:

react-docgen: 17 s for manager and 21 s for preview
react-docgen-typescript (before): 36 s for manager and 46 s for preview
react-docgen-typescript (after): 15 s for manager and 31 s for preview

mrmckeb · 2020-06-12T07:21:35Z

@hipstersmoothie It looks like this has introduced a regression, as react-docgen-typescript-plugin loads for all files (not just TypeScript), which means it is also injected for projects that aren't using TypeScript at all - and fails, as the typescript package is not found.

You can see here that we used to only target tsx? files. Is it possible to resolve this within the plugin?
https://github.com/storybookjs/storybook/pull/11106/files#diff-f65f40b676691502256f2800665754e1L38

hipstersmoothie · 2020-06-12T07:23:29Z

I actually just released an update with this functionality. 0.3.0 should only include tsx files.

mrmckeb · 2020-06-12T07:24:52Z

That's great news, and I'm guessing it only loads TypeScript as a dependency when ts and tsx files are found?

hipstersmoothie · 2020-06-12T07:27:39Z

Are you asking if I can conditionally require typescript if it’s installed? I’m assuming the loader only ever got loaded if a typescript file was found

mrmckeb · 2020-06-12T07:30:16Z

Yes, that was the previous behaviour. With this change, the plugin is always invoked - but we know that TypeScript may not be installed at all times. Do you conditionally require TypeScript now?

If not, we may need to revert this temporarily, as it will break in any projects that don't have a typescript dependency.

hipstersmoothie · 2020-06-12T07:49:28Z

PR incoming https://github.com/hipstersmoothie/react-docgen-typescript-plugin/releases/tag/v0.4.0

switch from react-docgen-typescript-loader to react-docgen-typescript…

4286a93

…-plugin

hipstersmoothie added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jun 9, 2020

hipstersmoothie requested review from alterx, igor-dv, ndelangen, shilman, thomasbertet and tmeasday as code owners June 9, 2020 21:28

hipstersmoothie removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jun 9, 2020

shilman added the typescript label Jun 9, 2020

shilman requested changes Jun 9, 2020

View reviewed changes

shilman added configuration babel / webpack core labels Jun 9, 2020

hipstersmoothie added 2 commits June 9, 2020 14:59

dont use babel plugin if using typescript docgen

09cbb9a

update perf stats

47f0f0d

shilman added this to the 6.0 typescript milestone Jun 9, 2020

shilman changed the title ~~switch from react-docgen-typescript-loader to react-docgen-typescript-plugin~~ React: Switch from react-docgen-typescript-loader to react-docgen-typescript-plugin Jun 9, 2020

shilman added react and removed core labels Jun 9, 2020

mrmckeb requested changes Jun 10, 2020

View reviewed changes

ndelangen approved these changes Jun 10, 2020

View reviewed changes

hipstersmoothie mentioned this pull request Jun 10, 2020

Add shouldExtractValuesFromUnion option styleguidist/react-docgen-typescript#261

Merged

shilman changed the title ~~React: Switch from react-docgen-typescript-loader to react-docgen-typescript-plugin~~ React: Switch react-docgen-typescript-loader to react-docgen-typescript-plugin Jun 11, 2020

shilman merged commit 62cd262 into storybookjs:next Jun 11, 2020

shilman added dependencies feature request and removed dependencies labels Jun 11, 2020

hipstersmoothie deleted the docgen-typescript branch June 11, 2020 15:41

This was referenced Jun 12, 2020

Storybook w/ Typescript takes ~2 minutes to load a story. #10784

Closed

Addon-controls: Better handling of Typescript optional properties #11146

Closed

mrmckeb mentioned this pull request Jun 15, 2020

fix: remove old TypeScript default config #11172

Closed

This was referenced Jun 16, 2020

Components: Remove PropsTable, clean ArgsTable stories #11105

Merged

React: Fix react-docgen for JS files #11217

Merged

lili2311 mentioned this pull request Jun 21, 2023

[Snyk-dev] Fix for 1 vulnerabilities lili2311/storybook#129

Open

lili2311 mentioned this pull request Sep 26, 2023

[Snyk-dev] Fix for 1 vulnerabilities lili2311/storybook#172

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React: Switch react-docgen-typescript-loader to react-docgen-typescript-plugin #11106

React: Switch react-docgen-typescript-loader to react-docgen-typescript-plugin #11106

hipstersmoothie commented Jun 9, 2020 •

edited

Loading

shilman left a comment

shilman commented Jun 9, 2020

hipstersmoothie commented Jun 9, 2020 •

edited

Loading

mrmckeb left a comment

mrmckeb Jun 10, 2020

hipstersmoothie Jun 10, 2020

mrmckeb Jun 11, 2020

mrmckeb Jun 10, 2020

ndelangen Jun 10, 2020

hipstersmoothie Jun 10, 2020

mrmckeb Jun 11, 2020

mrmckeb commented Jun 10, 2020

shilman commented Jun 10, 2020

hipstersmoothie commented Jun 10, 2020

hipstersmoothie commented Jun 10, 2020

hipstersmoothie commented Jun 10, 2020

hipstersmoothie commented Jun 10, 2020

shilman commented Jun 11, 2020

shilman commented Jun 11, 2020 •

edited

Loading

mrmckeb commented Jun 12, 2020 •

edited

Loading

hipstersmoothie commented Jun 12, 2020

mrmckeb commented Jun 12, 2020

hipstersmoothie commented Jun 12, 2020

mrmckeb commented Jun 12, 2020

hipstersmoothie commented Jun 12, 2020

React: Switch react-docgen-typescript-loader to react-docgen-typescript-plugin #11106

React: Switch react-docgen-typescript-loader to react-docgen-typescript-plugin #11106

Conversation

hipstersmoothie commented Jun 9, 2020 • edited Loading

What I did

How to test

shilman left a comment

Choose a reason for hiding this comment

shilman commented Jun 9, 2020

hipstersmoothie commented Jun 9, 2020 • edited Loading

skipPropsWithName skipPropsWithoutDoc

compilerOptions

mrmckeb left a comment

Choose a reason for hiding this comment

mrmckeb Jun 10, 2020

Choose a reason for hiding this comment

hipstersmoothie Jun 10, 2020

Choose a reason for hiding this comment

mrmckeb Jun 11, 2020

Choose a reason for hiding this comment

mrmckeb Jun 10, 2020

Choose a reason for hiding this comment

ndelangen Jun 10, 2020

Choose a reason for hiding this comment

hipstersmoothie Jun 10, 2020

Choose a reason for hiding this comment

mrmckeb Jun 11, 2020

Choose a reason for hiding this comment

mrmckeb commented Jun 10, 2020

shilman commented Jun 10, 2020

hipstersmoothie commented Jun 10, 2020

hipstersmoothie commented Jun 10, 2020

hipstersmoothie commented Jun 10, 2020

hipstersmoothie commented Jun 10, 2020

shilman commented Jun 11, 2020

shilman commented Jun 11, 2020 • edited Loading

mrmckeb commented Jun 12, 2020 • edited Loading

hipstersmoothie commented Jun 12, 2020

mrmckeb commented Jun 12, 2020

hipstersmoothie commented Jun 12, 2020

mrmckeb commented Jun 12, 2020

hipstersmoothie commented Jun 12, 2020

hipstersmoothie commented Jun 9, 2020 •

edited

Loading

hipstersmoothie commented Jun 9, 2020 •

edited

Loading

`skipPropsWithName` `skipPropsWithoutDoc`

`compilerOptions`

shilman commented Jun 11, 2020 •

edited

Loading

mrmckeb commented Jun 12, 2020 •

edited

Loading