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

Storybook loads stories from node_modules #19446

Closed
LucaColonnello opened this issue Oct 11, 2022 · 8 comments
Closed

Storybook loads stories from node_modules #19446

LucaColonnello opened this issue Oct 11, 2022 · 8 comments

Comments

@LucaColonnello
Copy link

Describe the bug
Storybook is loading stories from the node_modules folder.
This happens especially in monorepos, where usually packages have co-located stories.

This is responsible at times for crashes of the dev server as well as memory leaks or generally bad user experience due to how slow storybook becomes compiling, even when the number of stories is not that high.

Can node_modules be always ignored by default please? I am expecting this, but perhaps I'm wrong.
The issue definitely goes away listing the stories manually, even when the number of stories is above 100.

To Reproduce
After debugging storybook locally, I found out the issue here.
Essentially, the problem is linked to the fact that storybook is picking up stories from node_modules.

I believe this can be seen in a pnpm monorepo, as pnpm creates a nested node_modules folder structure.

In my case it was like this:

packages/
   my-pkg-1/
      stories.jsx
   my-pkg-2/      <- depends on my-pkg-1
      node_modules/
             my-pkg-1/
                   stories.jsx
      stories.jsx

So with a simple ./packages/**/stories.jsx pattern in the main.js file, all 3 stories files are loaded, rather than just 2.
If you think about a monorepo, this can happen a lot if node_modules are not hoisted.

System

Environment Info:

  System:
    OS: macOS 12.6
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
  Binaries:
    Node: 16.17.1 - ~/.nvm/versions/node/v16.17.1/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 8.15.0 - ~/.nvm/versions/node/v16.17.1/bin/npm
  Browsers:
    Chrome: 106.0.5249.91
    Firefox: 103.0.1
    Safari: 16.0
  npmPackages:
    @storybook/addon-essentials: ^6.5.12 => 6.5.12
    @storybook/addon-knobs: ^6.3.1 => 6.4.0
    @storybook/addons: ^6.5.12 => 6.5.12
    @storybook/api: ^6.5.12 => 6.5.12
    @storybook/builder-webpack5: ^6.5.12 => 6.5.12
    @storybook/components: ^6.5.12 => 6.5.12
    @storybook/core-events: ^6.5.12 => 6.5.12
    @storybook/manager-webpack5: ^6.5.12 => 6.5.12
    @storybook/react: ^6.5.12 => 6.5.12
    @storybook/storybook-deployer: ^2.8.12 => 2.8.12
    @storybook/theming: ^6.5.12 => 6.5.12

Additional context
This is mostly relevant to people using pnpm, but even when using yarn or npm, node_modules are not necessarily always hoisted, as if there are conflicts in resolution, specific versions would be installed locally for a package rather than hoisted.

I went further on investigating, and I can only see this behaviour in the StoryIndexGenerator usage of globby, but that seems to be only used when the storyStoreV7 feature is enabled. I think this is the same behaviour in the previous way to load stories (pre v7).

@LucaColonnello
Copy link
Author

LucaColonnello commented Oct 12, 2022

I tried the following pattern '../packages/*/(!(node_modules)**/)?*stories.{mdx,jsx}', and it's super slow with pnpm, as this translated to a require.context call internally (same pattern tested using picomatch works excluding the node_modules).

So I resorted to this to achieve compatibility with pnpm for monorepos:

// ./.storybook/main.jsx
const path = require('path');

const findStories = (includePattern, excludePattern) => async () => {
  const { globby } = await import('globby'); // globby is ESM only

  const storybookFolderRelativePaths = await globby([includePattern, `!${excludePattern}`], {
    cwd: path.join(process.cwd(), '.storybook'),
  });

  return storybookFolderRelativePaths;
};
module.exports = {
  core: {
    builder: 'webpack5',
  },
  stories: findStories('../packages/**/stories.{mdx,jsx}', '../packages/*/node_modules'),
  // ...
};

I'm happy to make a contribution to the docs to add this as an example of how to achieve full compatibility with pnpm in a monorepo setup, but I would like a response on whether this should be the default behaviour before assuming it should be fixed in the user config.

@jtbandes
Copy link

I'm experiencing this issue too. Any update on a fix for the root cause?

jtbandes added a commit to foxglove/studio that referenced this issue Apr 28, 2023
**User-Facing Changes**
None

**Description**
The changes here primarily affect internal contributing users.
The changes enable using TurboSnap with Chromatic, a visual regression
tool.

TurboSnap will optimize Chromatic build times and reduce the number of
snapshots used per triggered build.

Adding the `--webpack-stats-json` flag to the Storybook build script
enables the generation of a Webpack stats file, which Chromatic needs to
trace the Webpack dependency tree.

Adding `onlyChanged: true` to the Chromatic CI job enables TurboSnap.

The main reason for removing the `context` definition in the Webpack
configuration for Storybook is that it affected some internal
assumptions made by Storybook in generating the stats file.

This affects the relative path for the `moduleName` that Chromatic looks
for to resolve dependencies and find the relevant story changes.

Modifying the `makeConfig` should only affect the Storybook process and
leave the application Webpack build process unchanged.

storybookjs/storybook#19446 (comment)

I also removed the `findStories` function related to the open Storybook
issue above in favor of optimizing the stories glob pattern to exclude
`node_modules``.`

This speeds up the Storybook build time as well.

The trace now works correctly as well.

```
ℹ Traced 1 changed file to 3 affected story files:

— packages/studio-base/src/components/AppBar/UserMenu.tsx [changed]
  ∟ packages/studio-base/src/components/AppBar/UserMenu.stories.tsx
    ∟ [story index]

— packages/studio-base/src/components/AppBar/UserMenu.tsx [changed]
  ∟ packages/studio-base/src/components/AppBar/index.tsx + 4 modules
    ∟ packages/studio-base/src/Workspace.stories.tsx + 24 modules
      ∟ [story index]

— packages/studio-base/src/components/AppBar/UserMenu.tsx [changed]
  ∟ packages/studio-base/src/components/AppBar/index.tsx + 4 modules
    ∟ packages/studio-base/src/components/AppBar/UserMenu.stories.tsx [duplicate]
      ∟ [story index]

— packages/studio-base/src/components/AppBar/UserMenu.tsx [changed]
  ∟ packages/studio-base/src/components/AppBar/index.tsx + 4 modules
    ∟ packages/studio-base/src/components/AppBar/index.stories.tsx
      ∟ [story index]
```

---------

Co-authored-by: Jacob Bandes-Storch <jacob@foxglove.dev>
@Dammic
Copy link

Dammic commented May 25, 2023

Hey! I just wanted to say that this issue is still present with storybook@7.0.17 when using pnpm 8.5.1. @LucaColonnello solution worked for me perfectly, so thank you for that!

@V-iktor
Copy link

V-iktor commented Jun 8, 2023

I solved this by only including my src/ directories if it helps anyone:

"../../../packages/**/src/**/*.stories.@(js|jsx|ts|tsx)",

@LucaColonnello
Copy link
Author

Hey @V-iktor ,
That could ve another way yes, however, in a monorepo, the packages might be symlinked to the original files in node_modules when they depend on each other, meaning you'll get the node_modules src folders too with that glob pattern.

So in this particular instance, it might not solve the issue, unless you're hoisting everything to the root node_modules (or if you have no dependencies between packages in the same monorepo).

@V-iktor
Copy link

V-iktor commented Jun 9, 2023

I´m using Yarn Workspaces (Monorepo) and it seems to have solved the problems on my Mac at least

@LucaColonnello
Copy link
Author

Ah I see, so the issue was never with yarn, as yarn hoists node_modules at the root.

pnpm, on the contrary, uses a nested modules + symlinks approach, so this is why it wouldn't work...

@shilman
Copy link
Member

shilman commented Jun 12, 2023

Closed by #22873 and available in both the latest prerelease & latest stable release

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

No branches or pull requests

5 participants