Skip to content

Conversation

@ndelangen
Copy link
Member

@ndelangen ndelangen commented Jun 13, 2023

Replaces: #8

Testing in ecosystem: storybookjs/storybook#23040

📦 Published PR as canary version: 1.0.2--canary.12.cf35881ff63bf958b8998e8dd1dfd5626ee868c2.0

✨ Test out this PR locally via:

npm install @storybook/react-docgen-typescript-plugin@1.0.2--canary.12.cf35881ff63bf958b8998e8dd1dfd5626ee868c2.0
# or 
yarn add @storybook/react-docgen-typescript-plugin@1.0.2--canary.12.cf35881ff63bf958b8998e8dd1dfd5626ee868c2.0

@ndelangen ndelangen self-assigned this Jun 13, 2023
@ndelangen
Copy link
Member Author

ndelangen commented Jun 13, 2023

not good:
https://www.chromatic.com/test?appId=62f0fd349c07f78d10b7c017&id=64883ef76692a9984c1a89d6
https://www.chromatic.com/test?appId=634ff0d0ec053b270775979d&id=64883ef947b2da0adb278c26

@onigoetz would you be interested in helping figuring our what is causing these changes?

Here's my PR to storybook upgrading it to this canary: storybookjs/storybook#23040

note that storybook was using 1.0.6--canary.9.0c3f3b7.0 before.. maybe @kasperpeulen or @shilman will know?:
storybookjs/storybook@eb7ec7c

@onigoetz
Copy link

Hi, yes I'll try to have a look

@onigoetz
Copy link

Sorry for the delay ...

I had a look and found some surprising things so far :

The snapshot you posted ( https://www.chromatic.com/test?appId=62f0fd349c07f78d10b7c017&id=64883ef76692a9984c1a89d6 ) got me to find this code : https://github.com/storybookjs/storybook/blob/0eaaa8ccbebc446364019820669aba87444a95a3/code/renderers/react/template/stories/docgen-components/9493-ts-display-name/input.tsx

I added it to the plugin's fixtures and got the following result in the snapshot:

// types removed for brevity's sake

/**
 * This message should show up in the Docs panel if everything works fine.
 */
export const EmpireAlert: React.FC<EmpireAlertProps> = ({
  title = 'Code Yellow',
  message,
}: EmpireAlertProps) => (
  <div>
    <h1>{title}</h1>
    <p>{message}</p>
  </div>
);
EmpireAlert.displayName = 'SomeOtherDisplayName';

export const component = EmpireAlert;
try {
    // @ts-ignore
    SomeOtherDisplayName.displayName = \\"SomeOtherDisplayName\\";
    // @ts-ignore
    SomeOtherDisplayName.__docgenInfo = { \\"description\\": \\"This message should show up in the Docs panel if everything works fine.\\", \\"displayName\\": \\"SomeOtherDisplayName\\", \\"props\\": {} };
}
catch (__react_docgen_typescript_loader_error) { }"

I spot two issues here:

  1. __docgenInfo is assigned to SomeOtherDisplayName instead of EmpireAlert.
  2. Props are empty

I made some experiments by importing the library in various versions and generating the code and can't find a version of the library that doesn't have this behaviour, including the next branch. although that's weird because the snapshot must have been generated properly once ... maybe there is a cache somewhere I'm not aware of and one of the versions actually should work on my machine.

For the empty props issue though, changing the component to a function component yielded the following:

try {
    // @ts-ignore
    SomeOtherDisplayName.displayName = "SomeOtherDisplayName";
    // @ts-ignore
    SomeOtherDisplayName.__docgenInfo = { "description": "This message should show up in the Docs panel if everything works fine.", "displayName": "SomeOtherDisplayName", "props": { "title": { "defaultValue": { value: "Code Yellow" }, "description": "A title that brings attention to the alert.", "name": "title", "required": false, "type": { "name": "AlertCode" } }, "message": { "defaultValue": null, "description": "A message alerting about Empire activities.", "name": "message", "required": true, "type": { "name": "string" } } } };
}

Interestingly, this shows the default value, the possible values aren't resolved either. I will check how the upstream react-docgen-typescript behaves with these files

@onigoetz
Copy link

Digging the topic a bit further, I found that by

  1. setting the parser's shouldExtractValuesFromUnion option to true
  2. changing the Union.tsx's code like this
- export const EmpireAlert: React.FC<EmpireAlertProps> = ({
+ export const EmpireAlert = ({

The generated docgen is now

SomeOtherDisplayName.__docgenInfo = {
  description:
    "This message should show up in the Docs panel if everything works fine.",
  displayName: "SomeOtherDisplayName",
  props: {
    title: {
      defaultValue: { value: "Code Yellow" },
      description: "A title that brings attention to the alert.",
      name: "title",
      required: false,
      type: {
        name: "enum",
        value: [
          { value: '"Code Red"' },
          { value: '"Code Yellow"' },
          { value: '"Code Green"' },
        ],
      },
    },
    message: {
      defaultValue: null,
      description: "A message alerting about Empire activities.",
      name: "message",
      required: true,
      type: { name: "string" },
    },
  },
};

The assignment is still incorrect, but the types content is now resolved properly.

As a side effect, many other snapshots have more detailed types than they did before

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants