Yarn 2: Fix dependencies issues for compatibility #10613
Conversation
…erDeps of `@storybook/core`
- `babel-loader` is used in preset and require `@babel/core` - `@types/webpack` is needed because this package expose some `Configuration` type in generated preset.d.ts
- `core-js` is required because generated code imports a lot of things from it - `react-dom` is needed because this package is a peerDeps of `@storybook/addons` and `@storybook/client-api`
- `@storybook/core` is used to find `coreDirName` in `preset.ts` - `tslib` is needed because this package is a peerDeps of `@angular/core` - `@types/estree` is used in `acornParser` Also: - Remove `@ts-ignore`
Use `require.resolve` both for: - webpack loader - finding path, previously it was built assuming that `node_modules` directory exists which is false in Plug'n'PLay mode
`register` file of various addon should be resolve in the context of `addon-essentials` and not in the one of SB user projects
When generating an angular project using Angular CLI all these deps are not listed in `dependencies` so there is no reason for them to be in peerDependencies.
gaetanmaisse
requested review from
alterx,
CodeByAlex,
igor-dv,
jbovenschen,
thani-sh,
shilman,
thomasbertet and
tmeasday
as code owners
|
@ndelangen @kroeder I'm not 💯 sure about the last commit so comments are welcome! |
| @@ -112,7 +112,7 @@ export function webpack(webpackConfig: any = {}, options: any = {}) { | |||
| result.plugins.push( | |||
| new DllReferencePlugin({ | |||
| context, | |||
| manifest: path.join(coreDirName, 'dll', 'storybook_docs-manifest.json'), | |||
| manifest: require.resolve('@storybook/core/dll/storybook_docs-manifest.json'), | |||
There was a problem hiding this comment.
Does this need to use path.join for windows? Or does require.resolve handle that?
There was a problem hiding this comment.
That's a good question 🤔 I guess so, I based my change on what is done at L10: https://github.com/storybookjs/storybook/pull/10613/files#diff-3f9960d4367e0d7176bea0f6d79a54e7R10
@ndelangen maybe know the answer to that question?
I only have Unix computers so I can't directly test. However, the previous version was written by @tooppaaa and I think he is working on Windows so maybe he can give it a try and let us know if it works 😇. @tooppaaa if you have time can you:
git checkout fix-deps
yarn bootstrap --core
cd examples/official-storybook
yarn STORYBOOK_DISPLAY_WARNING=true DISPLAY_WARNING=true start-storybook -p 9011 -c ./Then let us know if SB has started properly and if stories related to addon docs are working fine. 🙇
There was a problem hiding this comment.
I think this works on windows, this would resolve to a path with the slashes in the wrong direction, as is how windows likes it. 🤷♂️
| @@ -66,7 +66,7 @@ export function webpack(webpackConfig: any = {}, options: any = {}) { | |||
| include: new RegExp(`node_modules\\${path.sep}acorn-jsx`), | |||
There was a problem hiding this comment.
I'm guessing this will do nothing in yarn 2?
| @@ -47,7 +47,6 @@ | |||
| }, | |||
| "peerDependencies": { | |||
| "@babel/core": "*", | |||
| "babel-loader": "^7.0.0 || ^8.0.0", | |||
There was a problem hiding this comment.
Nice! this means we can remove some documentation too, I think we tell users to install this?
shilman
changed the title
Related to Yarn 2 compatibility: #9527
Also related to #10315 as babel-loader is no more a peerDep
What I did
I cleaned some unneeded deps and added some missing ones. The missing deps were causing issues when running SB with Yarn 2.
You can review this PR commit by commit as I tried to explain the reason for the changes in each commit message.
With that PR all addons will work out of the box with Yarn 2 🎉!
How to test