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.

Sign up for GitHub

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

Jump to bottom

[Bug]: react-docgen-loader generates invalid code when re-exporting a component with a different name since 7.6 #26074

Open
glenjamin opened this issue Feb 16, 2024 · 10 comments · May be fixed by #26967
Open

[Bug]: react-docgen-loader generates invalid code when re-exporting a component with a different name since 7.6 #26074

glenjamin opened this issue Feb 16, 2024 · 10 comments · May be fixed by #26967
Labels
argtypes bug docgen react sev:S3 upgrade:7.6

Comments

@glenjamin
Copy link

Describe the bug

When defining a component in one file, but then re-exporting it with a different name in another file, react-docgen-loader generates invalid code which then errors when loading

In particular, the actualName of the docgen handler gets the __docgenInfo assigned, but in the re-importing file this name isn't present, leading to an "is not defined" error.

I haven't quite established whether this is a bug because react-docgen-loader should be adding the code differently, or because react-docgen should

To Reproduce

  1. Clone this demo repo https://github.com/glenjamin/storybook-repro-docgen
  2. npm i
  3. npm run storybook
  4. See that the story fails to render

The key parts of the example are

Which produces the following compiled code in the webpack bundle

/***/ "./src/components/index.js":
/*!*********************************!*\
  !*** ./src/components/index.js ***!
  \*********************************/
/***/ ((__unused_webpack_module, __webpack_exports__, __webpack_require__) => {

"use strict";
__webpack_require__.r(__webpack_exports__);
/* harmony export */ __webpack_require__.d(__webpack_exports__, {
/* harmony export */   Component: () => (/* reexport safe */ _Component__WEBPACK_IMPORTED_MODULE_0__["default"])
/* harmony export */ });
/* harmony import */ var _Component__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! ./Component */ "./src/components/Component.tsx");


;
MyComponent.__docgenInfo = {
  "description": "",
  "methods": [],
  "displayName": "MyComponent",
  "props": {
    "children": {
      "description": "",
      "type": {
        "name": "node"
      },
      "required": false
    }
  }
};

/***/ }),

System

Storybook Environment Info:

  System:
    OS: macOS 14.3
    CPU: (8) x64 Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
    Shell: 5.1.8 - /usr/local/bin/bash
  Binaries:
    Node: 18.8.0 - ~/.asdf/installs/nodejs/18.8.0/bin/node
    Yarn: 1.22.21 - ~/.asdf/installs/nodejs/18.8.0/bin/yarn
    npm: 8.18.0 - ~/.asdf/installs/nodejs/18.8.0/bin/npm <----- active
  Browsers:
    Chrome: 121.0.6167.184
    Edge: 121.0.2277.128
    Firefox: 122.0.1
    Safari: 17.3
  npmPackages:
    @storybook/addon-controls: ^7.6.16 => 7.6.16
    @storybook/addon-docs: ^7.6.16 => 7.6.16
    @storybook/addon-essentials: ^7.6.16 => 7.6.16
    @storybook/addon-interactions: ^7.6.16 => 7.6.16
    @storybook/addon-links: ^7.6.16 => 7.6.16
    @storybook/addon-onboarding: ^1.0.8 => 1.0.8
    @storybook/blocks: ^7.6.16 => 7.6.16
    @storybook/jest: ^0.2.3 => 0.2.3
    @storybook/react: ^7.6.16 => 7.6.16
    @storybook/react-webpack5: ^7.6.16 => 7.6.16
    @storybook/storybook-deployer: ^2.5.2 => 2.8.16
    @storybook/testing-library: ^0.2.2 => 0.2.2
    eslint-plugin-storybook: ^0.6.15 => 0.6.15
    storybook: ^7.6.16 => 7.6.16

Additional context

No response
@glenjamin
Copy link
Author

I tried to take a look into diagnosing/fixing this, but I couldn't find any tests for the docgen loader - are these covered by some e2e/integration suites? If anyone is able to give me a pointer on whereabouts to look to get a regression test set up that would be great.

@shilman
Copy link
Member

shilman commented Feb 23, 2024 •
edited

Hi @glenjamin!

We have TS test cases here: https://github.com/storybookjs/storybook/blob/next/code/renderers/react/template/stories/ts-argtypes.stories.tsx

And JS test cases here: https://github.com/storybookjs/storybook/blob/next/code/renderers/react/template/stories/js-argtypes.stories.jsx

Hope that helps!

@shilman
Copy link
Member

shilman commented Feb 23, 2024

Interestingly changing the docgen to react-docgen (an entirely different codepath) doesn't fix the problem. But disabling it does:

// main.ts
export default {
  typescript: {
    reactDocgen: false,
  }
}

@valentinpalkovic you touched all of this recently, any ideas? Given that this is broken in react-docgen-typescript (the default) I suspect this is not a 7.6 bug but something that's been around for awhile.

@glenjamin
Copy link
Author

Something that occurred to me while reading up on this is that react-docgen-loader is run on many files, and it also follows imports - so probably spends quite a lot of time parsing components that aren't used

I wonder if there's a way to only apply it to the components actually used in story files 🤔

@glenjamin
Copy link
Author

glenjamin commented Feb 23, 2024 •
edited

Interestingly changing the docgen to react-docgen (an entirely different codepath) doesn't fix the problem.

@valentinpalkovic you touched all of this recently, any ideas? Given that this is broken in react-docgen-typescript (the default) I suspect this is not a 7.6 bug but something that's been around for awhile.

From what I can see, the react-docgen and react-docgen-typescript settings both use @storybook/preset-react-webpack/dist/loaders/react-docgen-loader, which calls out to react-docgen under the hood - so I think the react-docgen upgrade in 7.6 is what introduced this bug

edit: aha, I see the docgen-loader is pretty new, it was only added in #24762

edit 2: I suspect it might be a bug that react-docgen-loader always uses react-docgen, regardless of the reactDocgen configuration option?

@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented Feb 23, 2024 •
edited

If react-docgen-typescript should be used, react-docgen still runs on non-TypeScript files since react-docgen-typescript can’t process JavaScript files, only TypeScript. See the test pattern here: https://github.com/storybookjs/storybook/blob/next/code/presets/react-webpack/src/framework-preset-react-docs.ts#L55. That's more or less the default behaviour like it was before.

@OleksandrKrupko
Copy link

totally valid, got the same issue

@is-paranoia
Copy link

got the same issue and it helps, but could this cause any problems in the future?

Interestingly changing the docgen to react-docgen (an entirely different codepath) doesn't fix the problem. But disabling it does:

// main.ts
export default {
  typescript: {
    reactDocgen: false,
  }
}

@valentinpalkovic you touched all of this recently, any ideas? Given that this is broken in react-docgen-typescript (the default) I suspect this is not a 7.6 bug but something that's been around for awhile.

@DBvc
Copy link

DBvc commented Apr 9, 2024

same issue

@domstepek
Copy link

Experiencing same issue here

@glenjamin glenjamin linked a pull request Apr 28, 2024 that will close this issue
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
argtypes bug docgen react sev:S3 upgrade:7.6
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Docgen: Only add react-docgen info when a component is defined in the file glenjamin/storybook
8 participants
@glenjamin @shilman @DBvc @valentinpalkovic @vanessayuenn @domstepek @OleksandrKrupko @is-paranoia