[8.0 QA] react-docgen as TS default

[vanessayuenn]

react-docgen as TS default

Feature owner: @shilman

Please comment in this discussion thread for any bug report and feedback for this feature as you test out Storybook 8. Kindly adhere to the following format:

Project: Name of your project as entered in Notion
Version: The version of storybook you tested with
Bug/feedback: Details of the bugs/feedback you have

Feature owner should triage the comments here frequently and open issues as needed.

[yannbf]

Project: MealDrop
Version: 8.0.0-alpha.14
Bug/feedback:

Types of the property typescript in main.ts are incompatible when using react-docgen.

Details and reproduction are provided in this issue:
#25736

[yannbf]

Project: MealDrop
Version: 8.0.0-alpha.15
Bug/feedback:

The automigration message has a few typos and seems confusing. At least to me, I was not sure what the migration wanted me to do. Seems more like a "do X or do Y" rather than a "yes/no" question to me.

which dramatically faster but doesn't handle all TypeScript constructs.

which is dramatically

though we recommend giving "react-docgen" for a much faster dev experience.

though we recommend giving "react-docgen" a try for a much faster dev experience.

Repro:

1. Add this to main.js:

typescript: {
    reactDocgenTypescriptOptions: {
      shouldExtractLiteralValuesFromEnum: true,
      propFilter: (prop) => {
        if (prop.parent) {
          return !prop.parent.fileName.includes('node_modules')
        }
        return true
      },
    }
  },

2. run npx storybook@next automigrate

[yannbf]

Project: MealDrop
Version: 8.0.0-alpha.14
Bug/feedback:

Not related to react-docgen but react-docgen-typescript. Seems like react-docgen-typescript does not have sensible defaults for its options. Is this intended?

For instance, without reactDocgenTypescriptOptions:

const config: StorybookConfig = {
  typescript: {
    reactDocgen: 'react-docgen-typescript',
  },
}

The types of all controls are inferred correctly

With an empty object reactDocgenTypescriptOptions (or another setting, e.g. shouldExtractLiteralValuesFromEnum: true):

const config: StorybookConfig = {
  typescript: {
    reactDocgen: 'react-docgen-typescript',
    reactDocgenTypescriptOptions: { },
  },
}

Notice that everything becomes "object"

I'd assume that users don't have to know which are the default values, so that if they set a single option such as shouldExtractLiteralValuesFromEnum, they would still get the previous behavior + the enum one

Repro:
It's hard because it will only happen once.

1. Go to https://github.com/yannbf/mealdrop/tree/storybook/8.0.0
2. Use the settings described above

[shilman]

if you want to merge a user-defined preset value with an existing preset you should define a preset function and merge it yourself:

export const typescript = (existing) => ({
  ...existing,
  reactDocgenTypescriptOptions: {
    ...existing?.reactDocgenTypescriptOptions,
    /* some options here */
  },
});

It wouldn't be hard to change this so that the default options are empty and then the merge happens at the place where the options are consumed. However, this would be non-standard and also suck if we wanted to consume the data in various places.

@ndelangen @yannbf do you have any thoughts on this? It seems like basically a complaint about how presets work.

[ndelangen]

@shilman I'd say no to changing this. How WOULD you remove a default property then?

[yannbf]

I think it makes sense to merge the options. However, the docs never mention anything regarding merging options, so I would assume users would always fall into this issue. It might seem obvious to the Storybook team but I think users might not really understand that this is the way to go. cc @kylegach WDYT?

[ndelangen]

Here's the default:

storybook/code/lib/core-server/src/presets/common-preset.ts

Lines 140 to 146 in 5e3532d

	reactDocgenTypescriptOptions: {
	shouldExtractLiteralValuesFromEnum: true,
	shouldRemoveUndefinedFromOptional: true,
	propFilter: (prop: any) => (prop.parent ? !/node_modules/.test(prop.parent.fileName) : true),
	// NOTE: this default cannot be changed
	savePropValueAsString: true,
	},

but we don't deeply merge when presets values are objects.