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 monorepo compatibility #11753

Merged
merged 14 commits into from
Sep 11, 2020
Merged

Core: Fix monorepo compatibility #11753

merged 14 commits into from
Sep 11, 2020

Conversation

merceyz
Copy link
Contributor

@merceyz merceyz commented Aug 1, 2020

Issue

Storybook doesn't work correctly in monorepos with both node_modules (Yarn 1/2) and PnP (Yarn 2) because it's trying to require the addons declared in .storybook/main.js from @storybook/core instead of requiring them on behalf of the config.

The e2e tests currently pass because Yarn has a compatability feature, called fallbackMode, that allows dependencies to require top-level dependencies without declaring them as their own. In a monorepo this fails as the top level no longer declares these dependencies, the workspace does, and under the node_modules linker the addons are not guaranteed to be hoisted to the same level as @storybook/core.

What I did

How to test

Disable the fallbackMode and try to use storybook in a PnP project

Extra

cc @gaetanmaisse since you worked on PnP compatability

@gaetanmaisse
Copy link
Member

Thanks @merceyz, this looks interesting! I will deep dive in this when I will come back from holidays in 10 days.

@merceyz merceyz changed the title ci: disable fallback mode in yarn 2 tests fix: correct resolving in monorepos Aug 14, 2020
@merceyz merceyz marked this pull request as draft August 14, 2020 15:42
merceyz and others added 4 commits September 11, 2020 12:40
Storybook doesn't work correctly in monorepos with both node_modules (Yarn 1/2) and PnP (Yarn 2) because it's trying to require the addons declared in .storybook/main.js from @storybook/core instead of requiring them on behalf of the config.

Also makes checks on preset names less restrictive as they are now resolved path and not simple name anymore.
@gaetanmaisse gaetanmaisse added maintenance User-facing maintenance tasks yarn / npm Yarn / npm acting weird labels Sep 11, 2020
@gaetanmaisse
Copy link
Member

@merceyz All green! 🎉 Do you have something to add or we can open the PR?

@merceyz merceyz marked this pull request as ready for review September 11, 2020 11:56
@merceyz
Copy link
Contributor Author

merceyz commented Sep 11, 2020

@gaetanmaisse Nope, all good, thanks for finding the issue with the CRA preset 🎉

@gaetanmaisse gaetanmaisse changed the title fix: correct resolving in monorepos Core: Fix Yarn monorepo compatibility Sep 11, 2020
@merceyz
Copy link
Contributor Author

merceyz commented Sep 11, 2020

Note to whomever merges this: You should use Squash and Merge since the commit history is a bit messy here


@gaetanmaisse NIT on the title: This isn't exclusive to Yarn monorepos, it fixes monorepos using pnpm / npm + lerna as well

@gaetanmaisse gaetanmaisse changed the title Core: Fix Yarn monorepo compatibility Core: Fix monorepo compatibility Sep 11, 2020
@shilman
Copy link
Member

shilman commented Sep 11, 2020

@merceyz no worries about the commit history. we don't use squash merges in this project! 😁

@shilman shilman added this to the 6.1 maintenance milestone Sep 11, 2020
@shilman shilman merged commit df297e2 into storybookjs:next Sep 11, 2020
@merceyz
Copy link
Contributor Author

merceyz commented Sep 11, 2020

we don't use squash merges in this project

That makes the commit history really hard to read but alright

@khats
Copy link

khats commented Nov 25, 2020

How to resolve preset relative to storybook installed folder, but not config?
there is following issue after resolving preset relative to config:
storybook installed under ./build/storybook . one of monorepo packages locates in ./monorepo-pack1 and has ./monorepo-pack1/./storybook/main.js with enabled addon: @storybook/addon-essentials . There is symlink to start-storybook from ./monorepo-pack1 to ./build/storybook/node_modules/@storybook/react .

After changes in this pull request storybook tries to look ./monorepo-pack1/node_modules/@storybook/addon-essentials, but expected for me is ./build/storybook/node_modules/@storybook/addon-essentials where is storybook and other addons installed.
I want to have different configurations for monorepo for each sub project, but don't want to add @storybook/addon-essentials for each project as devDependency

@merceyz
Copy link
Contributor Author

merceyz commented Nov 25, 2020

Resolve the presets upfront yourself using require.resolve

@khats
Copy link

khats commented Nov 25, 2020

@merceyz thank your for answer
After changing addon name in main.js from @storybook/addon-essentials to require.resolve(path.join(__dirname,"../../build/storybook/node_modules/@storybook/addon-essentials")) there is no more error Addon value should end in /register OR it should be a valid preset and addon(viewports) works, but i have warning
Expected '@storybook/addon-docs' to be listed before '@storybook/addon-controls' (or '@storybook/addon-essentials'). Check your main.js? https://github.com/storybookjs/storybook/issues/11442

@khats
Copy link

khats commented Nov 25, 2020

in case of using addon-essentials with custom full path like: require.resolve("../..build/storybook/node_modules/@storybook/addon-essentials") ensureDocsBeforeControls fails because expects that name of the addon will start with "@storybook/addon-essentials". Looks like fix could be not using name.startsWith(addon) for finding addon, but name.includes(addon)

upd: i have found same implementation already done
d5efc50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core maintenance User-facing maintenance tasks yarn / npm Yarn / npm acting weird
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants