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

Core: Fix indexing errors by excluding node_modules stories #22873

Merged
merged 2 commits into from
Jun 6, 2023

Conversation

shilman
Copy link
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
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
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
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
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 patch:yes Bugfix & documentation PR that need to be picked to main 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
3 participants