storybookjs / presets

🧩 Presets for Storybook
MIT License
424 stars 104 forks source link

preset-create-react-app: Fix monorepos and PnP #181

Closed merceyz closed 3 years ago

merceyz commented 3 years ago

What's the problem this PR addresses?

Fixes https://github.com/storybookjs/presets/issues/58 Fixes https://github.com/storybookjs/presets/issues/161 Fixes https://github.com/storybookjs/presets/issues/168 Fixes https://github.com/storybookjs/storybook/issues/10078 Closes https://github.com/storybookjs/presets/pull/166

How did you fix it?

merceyz commented 3 years ago

The logic here https://github.com/storybookjs/presets/blob/0a8ce5616fd9a9a77b3296d151221152c07dbbff/packages/preset-create-react-app/src/helpers/getReactScriptsPath.ts#L5-L43 should be removed in the next major and instead just have users provide the forked name as an option, which is already supported and doesn't rely on implementation details of the package managers

gaetanmaisse commented 3 years ago

cc @shilman, a.k.a. release master, for your comment about the cleaning that would require a major release

gaetanmaisse commented 3 years ago

@shilman can we merge and release this one?

I think it could solve some of the current issues in https://github.com/storybookjs/storybook/pull/13559 😇

rdsedmundo commented 3 years ago

I'm using pnpm and I had to specify options.scriptsPackageName='react-scripts', even though it's not a fork. This is because the symlink resolution doesn't work the same way for pnpm, it's not a symlink in their case, and the realpath resolution resolves to the file itself, causing the module to not be found.

The #166 PR had removed this logic altogether, this PR didn't touch it but it closed #166 due to the "Closes" in the description.

I see @merceyz mentioned the necessity of removing this logic on the next major release, is this tracked in an issue? Otherwise, it'll likely be forgotten.