Skip to content

Core: Fix indexing errors by excluding node_modules stories#22873

Merged
shilman merged 2 commits into
nextfrom
shilman/21414-exclude-node-modules
Jun 6, 2023
Merged

Core: Fix indexing errors by excluding node_modules stories#22873
shilman merged 2 commits into
nextfrom
shilman/21414-exclude-node-modules

Conversation

@shilman
Copy link
Copy Markdown
Member

@shilman shilman commented Jun 1, 2023

Closes #21414 #19446

What I did

Exclude stories from node_modules from the story index. Starting in 7.0, we are including story files in the Storybook renderer packages, which are installed into node_modules. This causes errors for users whose stories globs are configured to match node_modules. Also matching node_modules is a huge performance hog.

Now if you want to include stories from node_modules you need to explicitly add that to your globs. This is described in my change to MIGRATION.md.

This is technically a breaking change but I expect it to break very few users, and the problems that it fixes were introduced in 7.0, which is still stabilizing. Hence I think it is safe to patch back.

How to test

In a sandbox:

  1. Add a file node_modules/Dummy.jsx containing the following content:
import React from 'react'
const Dummy = () => <div>Dummy</div>
export default { title: 'Dummy', component: Dummy }
export const Default = {};
  1. Update the main.js to include the pattern ../**/*.stories.@(js|jsx|ts|tsx).
  2. Verify that Dummy does not appear in the sidebar
  3. Replace the exclude pattern in common-glob-options.ts (added in this PR) from '/node_modules/'to'/@storybook/'`
  4. Verify that Dummy appears at the top of the sidebar

Perform all of the above:

  • In a vite sandbox
    • Normally
    • In SSv6 mode by setting features.storyStoryV7 = true and features.storyStoreV7MdxErrors = false
  • In a webpack sandbox
    • Normally
    • In SSv6 mode

@shilman shilman added bug BREAKING CHANGE patch:yes Bugfix & documentation PR that need to be picked to main branch core labels Jun 1, 2023
@shilman shilman self-assigned this Jun 1, 2023
Copy link
Copy Markdown
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable approach. I haven't QA-ed it yet.

Copy link
Copy Markdown
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested with both: '../**/*.stories.@(js|jsx|ts|tsx)',and '../node_modules/*.stories.@(js|jsx|ts|tsx)' in all 4 environments 🚀

@yannbf
Copy link
Copy Markdown
Member

yannbf commented Jun 6, 2023

This looks fantastic!

@shilman shilman merged commit 821e93a into next Jun 6, 2023
@shilman shilman deleted the shilman/21414-exclude-node-modules branch June 6, 2023 08:10
shilman added a commit that referenced this pull request Jun 6, 2023
…-modules

Core: Fix indexing errors by excluding node_modules stories
@shilman shilman mentioned this pull request Jun 7, 2023
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 8, 2023
@ndelangen ndelangen removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug core patch:done Patch/release PRs already cherry-picked to main/release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Unable to index files: Duplicate stories with id: example-button--primary

4 participants