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

React: Set react-docgen to default TS docgen #24165

Merged
merged 23 commits into from
Nov 30, 2023

Conversation

shilman
Copy link
Member

@shilman shilman commented Sep 13, 2023

Telescopes on #23825

What I did

Set react-docgen as the default so we can look at Chromatic diffs easily. Won't merge until:

Also needs:

  • Automigration

Checklist for Contributors

Testing

  • See attached Chromatic diffs
  • Will add more test cases TBD

🦋 Canary release

This pull request has been released as version 0.0.0-pr-24165-sha-acaa9f93. Install it by pinning all your Storybook dependencies to that version.

More information
Published version 0.0.0-pr-24165-sha-acaa9f93
Triggered by @yannbf
Repository storybookjs/storybook
Branch shilman/default-to-react-docgen
Commit acaa9f93
Datetime Mon Oct 23 07:43:10 UTC 2023 (1698046990)
Workflow run 6610198728

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=24165

Base automatically changed from shilman/fix-react-docgen-enum to next October 4, 2023 04:37
@shilman shilman changed the base branch from next to release-8-0 October 4, 2023 04:40
@shilman shilman changed the title React: Set react-docgen as the default React: Set react-docgen as default docgen Oct 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

Fails
🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against 1fa44fc

code/addons/docs/react/README.md Outdated Show resolved Hide resolved
code/lib/core-server/src/presets/common-preset.ts Outdated Show resolved Hide resolved
@shilman shilman added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:normal labels Nov 3, 2023
@csantos1113
Copy link

I'm curious what is the foundational difference between react-docgen and react-docgen-typescript and when I should use one or another?

@ndelangen
Copy link
Member

@csantos1113 react-docgen is a lot faster, because it doesn't use tsc-APIs.

But those tsc-APIs provide output akin to the actual types, whereas react-docgen's output can be less correct.

@ndelangen
Copy link
Member

@shilman can this PR proceed?

@shilman
Copy link
Member Author

shilman commented Nov 28, 2023

Not yet. There are still some chromatic diffs that I'd like to investigate. @ndelangen

@github-actions github-actions bot deleted the branch next November 28, 2023 23:23
@github-actions github-actions bot closed this Nov 28, 2023
@JReinhold JReinhold reopened this Nov 29, 2023
@JReinhold JReinhold changed the base branch from release-8-0 to next November 29, 2023 07:38
@csantos1113
Copy link

@csantos1113 react-docgen is a lot faster, because it doesn't use tsc-APIs.

But those tsc-APIs provide output akin to the actual types, whereas react-docgen's output can be less correct.

@ndelangen - Thanks for the reply; I changed it to react-docgen and while it's significantly faster, I'm losing lot of useful context from the types and inline docs

left is react-docgen-typescript, right is react-docgen

Screenshot 2023-11-29 at 11 45 49 AM Screenshot 2023-11-29 at 11 46 30 AM

One of these props in code:
image


Sorry for hijacking this PR with this question, I could also open a issue about this topic, but I think it's sort of relevant for this PR because the default behavior will change in SB 8.0, if we make react-docgen the default

@valentinpalkovic
Copy link
Contributor

@csantos1113 Can you tell us with which Storybook version you have tried this and whether you are using a webpack5 or vite based project?

@csantos1113
Copy link

csantos1113 commented Nov 29, 2023

@csantos1113 Can you tell us with which Storybook version you have tried this and whether you are using a webpack5 or vite based project?

Certainly @valentinpalkovic !

  • storybook packages: 7.6.1
  • using @storybook/react-vite
  • my codebase is written in typescript

Here a simplified code example:
image

types.ts

export enum ButtonType {
  Button = 'button',
  Reset = 'reset',
  Submit = 'submit'
}

export type ButtonTestProps = {
  /**
   * Optional button type for the component
   */
  type?: ButtonType;
};

index.tsx

import { ButtonTestProps } from './types';

export const ButtonTest = (props: ButtonTestProps) => {
  return <button {...props}>Test</button>;
};

index.stories.ts

import { Meta, StoryObj } from '@storybook/react';

import { ButtonTest } from '.';
import { ButtonType } from './types';

const meta = {
  args: {
    type: ButtonType.Button
  },
  component: ButtonTest
} satisfies Meta<typeof ButtonTest>;
export default meta;

type Story = StoryObj<typeof meta>;

export const Default: Story = {};

✅ 😍 Results with react-docgen-typescript

image

🤕 Results with react-docgen

image

Notice the "Description" and "Control"

😞 And it's even worst if I use path aliases (for example ~/button-test/ instead of ./) on the imports

image

Notice the "Description" is gone

Diff between react-docgen and react-docgen-typescript

I understand there are foundational differences between the two libraries, and react-docgen has a bunch of open TypeScript related issues: https://github.com/reactjs/react-docgen/issues?q=is%3Aissue+is%3Aopen+typescript


It is understandable that making these two libraries work "the same" is not responsibility of the Storybook team; but if Storybook changes the default to react-docgen, a lot of typescript users like myself will be impacted; so perhaps a warning/notification should be added somewhere in the migration guides at a minimum.

Note: I know 8.0 is in is early stages, just adding my early feedback too 😅

UPDATE

I re-read the MIGRATION.md file and all the code changes in here, I think this is the best we would get given the differences between these two libraries (and I understand the decision behind it)

@shilman
Copy link
Member Author

shilman commented Nov 30, 2023

@csantos1113 Thank you so much for raising this. It's exactly the kind of feedback we're looking for. I've confirmed that this is indeed a bug in react-docgen (not in SB's handling of the react-docgen results). Any chance you could file a separate issue about this with a repro from https://storybook.new?

This looks like a common case -- my guess is there will be a few high priority bugs like this and fixing them will give us an 80/20 solution. We will rely on the community to fix them and intervene to fix ourselves if that doesn't happen quickly enough.

Then there will be a long tail of corner cases that we probably won't address. I see react-docgen as a temporary solution and we'd have a TypeScript based solution that is more robust, but processes the data asynchronously so as not to affect Storybook startup times. But that's a bigger project and we're focused on other stuff right now.

@shilman shilman changed the title React: Set react-docgen to default docgen React: Set react-docgen to default TS docgen Nov 30, 2023
@shilman shilman merged commit 2e4e691 into next Nov 30, 2023
101 of 104 checks passed
@shilman shilman deleted the shilman/default-to-react-docgen branch November 30, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
argtypes BREAKING CHANGE ci:daily Run the CI jobs that normally run in the daily job. react typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants