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

Addon-docs: Resolve babel-loader from storybook/core #13607

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

shilman
Copy link
Member

@shilman shilman commented Jan 11, 2021

Issue: #13593

What I did

Resolve babel-loader from @storybook/core so we don't need to install it and potentially break CRA. This is a workaround for what appears to be a yarn bug.

How to test

CI build

self-merging @tooppaaa @mrmckeb

@shilman shilman added bug yarn / npm Yarn / npm acting weird addon: docs configuration babel / webpack cra Prioritize create-react-app compatibility patch:yes Bugfix & documentation PR that need to be picked to main branch labels Jan 11, 2021
@shilman shilman added this to the 6.1.x milestone Jan 11, 2021
@shilman shilman merged commit e9ef268 into next Jan 11, 2021
@shilman shilman deleted the 13593-babel-loader-from-core branch January 11, 2021 18:39
@merceyz
Copy link
Contributor

merceyz commented Jan 11, 2021

This is a workaround for what appears to be a yarn bug.

If the parent of @storybook/addon-docs doesn't declare babel-loader as a dependency then this is expected

@shilman
Copy link
Member Author

shilman commented Jan 11, 2021

@merceyz what's the parent of @storybook/addon-docs?

when i do yarn list it shows babel-loader at the root, but when i look in the node_modules, babel-loader is actually located inside subprojects (unhoisted), so definitely something fishy going on.

shilman added a commit that referenced this pull request Jan 11, 2021
Addon-docs: Resolve babel-loader from storybook/core
@gaetanmaisse
Copy link
Member

what's the parent of @storybook/addon-docs?

@shilman it's either the user's project or @storybook/addon-essential.

I think everything was working fine because sb init was adding babel dependencies before #13220 has been merged.

@shilman
Copy link
Member Author

shilman commented Jan 12, 2021

Thanks for clarifying. If that's the case I'm surprised we're only hearing about this error now, unless I missed it (very possible!). Either way, this seems to fix the problem. Is there a more proper way to do it?

Copy link
Member

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

Amazing work @shilman!

@merceyz
Copy link
Contributor

merceyz commented Jan 12, 2021

@merceyz what's the parent of @storybook/addon-docs?

Running yarn create react-app foo && cd foo && yarn dlx sb init shows that it's the users project, which storybook stopped installing babel-loader into in #13220, so it not working makes perfect sense, PnP would have stopped this one but that isn't covered in the e2e test atm

yarn why babel-loader -R
└─ foo@workspace:.
   ├─ @storybook/addon-docs@npm:6.1.11 [6847a] (via npm:^6.1.11 [6847a])
   │  └─ @storybook/core@npm:6.1.11 [25a5a] (via npm:6.1.11 [25a5a])
   │     └─ babel-loader@npm:8.2.2 [bb165] (via npm:^8.0.6 [bb165])
   ├─ @storybook/addon-essentials@npm:6.1.11 [6847a] (via npm:^6.1.11 [6847a])
   │  └─ @storybook/addon-docs@npm:6.1.11 [22d5d] (via npm:6.1.11 [22d5d])
   │     └─ @storybook/core@npm:6.1.11 [25a5a] (via npm:6.1.11 [25a5a])
   ├─ @storybook/react@npm:6.1.11 [6847a] (via npm:^6.1.11 [6847a])
   │  └─ @storybook/core@npm:6.1.11 [25a5a] (via npm:6.1.11 [25a5a])
   └─ react-scripts@npm:4.0.1 [6847a] (via npm:4.0.1 [6847a])
      └─ babel-loader@npm:8.1.0 [7f8b9] (via npm:8.1.0 [7f8b9])

Is there a more proper way to do it?

It's perfectly fine to resolve dependencies from one of your dependencies as long as you know they'll continue to depend on it to not suddenly break in a patch release. That said you shouldn't have to, create-react-app shouldn't care what is in node_modules as the package manager places everything they need where it needs to be either way.

when i do yarn list it shows babel-loader at the root, but when i look in the node_modules, babel-loader is actually located inside subprojects (unhoisted), so definitely something fishy going on.

It's not hoisted to the root because of dependency conflicts and you can't rely on it to be hoisted either (hello PnP), if you look at the output above you see the actual dependency graph and based on just that @storybook/addon-docs doesn't have access

@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jan 12, 2021
@shilman
Copy link
Member Author

shilman commented Jan 12, 2021

thanks so much for the clarification @merceyz i'm a little bit slow on all this, but i appreciate your patience. we'll get there! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs bug configuration babel / webpack cra Prioritize create-react-app compatibility 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 yarn / npm Yarn / npm acting weird
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants