-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Fix NPM7 peer dependencies #14119
Comments
@shilman i still have issues...
|
@stevendavisfoto That's why the issue is still open. It hasn't been resolved yet. |
@Methuselah96 what's the solution for now? downgrade to react 16? use |
Yes, either use |
I'd like to contribute and replace |
@rodrigoehlers Thanks for your enthusiasm to fix this!! I was probably referring to this: https://reacttraining.com/blog/reach-react-router-future/ @ndelangen did the Let me know what you think and feel free to DM on Discord: https://discord.gg/storybook |
@shilman Sounds good, thanks. I'll wait for input from @ndelangen and setup the development environment in the meantime. If I don't contribute with this, I'll definitely do with another issue. |
@rodrigoehlers I just chatted with @ndelangen and he's cool with the project. Please hit us up on Discord if you have questions getting set up or need a hand in any way! |
Would changing the storybook/addons/actions/package.json Lines 62 to 65 in 8a40dbd
to {
"peerDependencies": {
"react": ">=16.8.0",
"react-dom": ">=16.8.0"
}
} Solve the npm 7 install issue? |
@andykenward but storybook IS compatible with react 17 at runtime. Won't changing that mean users will get a warning? Seems to me there's simply no point in having peerDependencies in npm 7? |
@ndelangen NPM 7 installs peerDependencies for you by default. It even adds them to your projects The fix in #14106 for the cli sb only works on the initial usage. Making my suggested change #14119 (comment) thought-out the storybook repo and testing locally takes a bit of time. Got stuck with the local publish process to verdaccio failing for a couple npm packages. Not had a chance to spend more time on it. |
@andykenward I appreciate your expertise and time invested! I personally haven't used npm's CLI in a long time. Can you explain to me why B is better and actually solves the problem?:
Can you help me understand how come npm seems to choose the incorrect version then? Why with version-range B, does npm chose the correct one?
Understandable, again I want to stress, I'm really appreciative of our time! |
I think this recent conversation is barking up the wrong tree. There are two issues that are clearly described in the desciption. The first issue is with external dependencies not allowing React 17 (e.g., The peer dependency range for |
@Methuselah96 I am going by the error message from npm 7. Which is flagging that the example project {
"name": "vite-project",
"version": "0.0.0",
"scripts": {
"dev": "vite",
"build": "tsc && vite build",
"serve": "vite preview",
"storybook": "start-storybook -p 6006",
"build-storybook": "build-storybook"
},
"dependencies": {
"react": "^17.0.0",
"react-dom": "^17.0.0"
},
"devDependencies": {
"@babel/core": "^7.13.15",
"@storybook/addon-actions": "^6.3.0-alpha.3",
"@storybook/addon-essentials": "^6.3.0-alpha.3",
"@storybook/addon-links": "^6.3.0-alpha.3",
"@storybook/react": "^6.3.0-alpha.3",
"@types/react": "^17.0.0",
"@types/react-dom": "^17.0.0",
"@vitejs/plugin-react-refresh": "^1.3.1",
"babel-loader": "^8.2.2",
"typescript": "^4.1.2",
"vite": "^2.1.5"
}
} I was just seeing if it would be an easier fix for the addon packages by doing my suggested change. |
@andykenward Right, the top part of that error message under "Found [email protected]" is specifying the React dependencies that are correctly resolved to React 17 (which includes |
Ah okay. It looks like the storybook/lib/router/package.json Line 39 in 90f27eb
|
@Methuselah96 D'oh I totally missed the difference between "Found [email protected]" vs "Could not find resolve dependency". I just saw the red |
I started an issue specifically for reach router to react-router 6: #14619 I gave it a shot this morning but gave up after a few hours. Perhaps this would be easier for someone more familiar with @storybook/router and the various imported types around @reach/router. The quickest path forward would be for reach to just support reach 17: reach/router#452 It's unclear if the maintainers are willing to do that, if anyone knows they are not, I can stop bugging them about it 😆 . |
I guess that's me! I'd be happy to debug this together, want to schedule something? |
Any update one this? |
@ndelangen why not migrate to react-router 6. happy to take a look I know it's in beta but it's pretty stable now. We are about to use in in production |
Sorry got busy at work last couple of weeks. Simplest thing is to fork reach and publish a scoped fork that supports react > 16. Second to that doing the work to upgrade to react-router 6 but that requires some non-trivial refactoring. |
Ok so gonna try and starat this then. |
…) has a peer-dep on React 16.x which conflicts with PRC’s peer-dep on React 17.x" This reverts commit e3612051ab01bf1dfbdf477f00ef96b0ea8aca47. Storybook has the same issue (storybookjs/storybook#14119, storybookjs/storybook#14619), so if we’re open to using '--legacy-peer-deps' to use Storybook, we can’t justify removing Playroom on anti-'--legacy-peer-deps' grounds.
There has been some replies on the linked #reach/router/452 PR related to this issue. It is indeed suggested to switch momentarily to a fork for react 17 support. It seems that the reach-router is not maintained anymore. The last PR merged was made by dependabot 9 months ago. |
Just curious, @Jordan-Hall, since version 6 has been in beta for a long time without movement, would it maybe be safer to move first to react-router v5, and then move to 6 once it's stable? |
We recently upgraded to react-router v6!! |
Hi guys. I'm using 2 nextJS applications in a monorepo, which all of them use react 18. |
Are you able to run |
@ricardo-fnd what do you mean by "I can't install the supported react version"? Can you share the warnings you're seeing? react-router supports any react >= 16.8, so maybe you're having a different issue? |
@IanVS peerDeps from storybook @Methuselah96 I am able yes. Will storybook just use the react version that I'm using (v18.0.0)? |
I don't believe the latest stable version (i.e., |
It's "compatible" in the sense that it will render, but you'll see warnings in your console, and react 18 features won't work. @ricardo-fnd do you have react and react-dom installed in the workspace where your storybook is installed? The warnings make it seem like you might not. |
@Methuselah96 pre-release solved most of the warnings, but some modules still not support react 18 even in pre-release. maybe wip? |
@IanVS I have react v.18.0.0 |
@ricardo-fnd i don't believe SB works with react 18 yet. it only came out last month. |
It should work when using the latest alpha versions of 6.5: #17215 |
Fixed in 7.0 beta |
EDIT: Decided to file a new ticket for this. It's here: #21396 Just installed the latest storybook 7 version in our ERR_PNPM_PEER_DEP_ISSUES Unmet peer dependencies
_tooling/storybook
└─┬ @storybook/blocks 7.0.0-alpha.8
└─┬ @mdx-js/react 1.6.22
└── ✕ unmet peer react@"^16.13.1 || ^17.0.0": found 18.2.0 Not sure why In #20260 it was said that mdx was updated in december (?) |
@D1no please try the latest beta instead, alpha.8 is quite old already. |
It looks like alpha.8 has the |
NPM7 has changed the semantics of peer dependencies which caused NPM to fail: #12983
I updated the CLI to use
--legacy-peer-deps
to work around this issue: #14106However, the underlying structure of the project has not changed, and presumably this will bite us in the future.
There are at least two kinds of dependency errors:
@reach/router
which are not React17 compatibleaddon-docs
External libraries
For each library that is not NPM7 compatible, we should either
In the case of
@reach/router
, the path forward could be React Router which does support React 17Internal dependencies
@storybook/addon-docs
has optional peer dependencies on each of the framework packages it supports@storybook/react
,@storybook/vue
, etc. It should be refactored to use dependency injection so that those dependencies can be removed. In addition, the react-specific code should probably be moved into@storybook/react
, which is a bigger project.The text was updated successfully, but these errors were encountered: