storybook icon indicating copy to clipboard operation
storybook copied to clipboard

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

Open glenjamin opened this issue 1 year ago • 1 comments

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

  • The component definition: https://github.com/glenjamin/storybook-repro-docgen/blob/main/src/components/Component.tsx
  • The re-exporting file https://github.com/glenjamin/storybook-repro-docgen/blob/main/src/components/index.js

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 avatar Feb 16 '24 18:02 glenjamin

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.

glenjamin avatar Feb 19 '24 10:02 glenjamin

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 avatar Feb 23 '24 00:02 shilman

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.

shilman avatar Feb 23 '24 02:02 shilman

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 avatar Feb 23 '24 08:02 glenjamin

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 https://github.com/storybookjs/storybook/pull/24762

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

glenjamin avatar Feb 23 '24 18:02 glenjamin

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.

valentinpalkovic avatar Feb 23 '24 19:02 valentinpalkovic

totally valid, got the same issue

OleksandrKrupko avatar Mar 19 '24 21:03 OleksandrKrupko

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.

is-paranoia avatar Mar 21 '24 05:03 is-paranoia

same issue

DBvc avatar Apr 09 '24 03:04 DBvc

Experiencing same issue here

domstepek avatar Apr 18 '24 15:04 domstepek