-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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(remix-dev): fix config call when using Yarn PnP #3566
Conversation
Hi @lensbart, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
"@cloudflare/workers-types": "^2.0.0 || ^3.0.0", | ||
"react": ">=16.8", | ||
"react-dom": ">=16.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already had a couple of PRs doing this and we decided we don't want this as these aren't necessary in any way.
This is only there because we're relying on react-router
, which has these in peerDependencies
as well.
Once we're switching to @remix-run/router
(once react-router
v6.4 is released), these won't be necessary anymore in peerDependencies
, so all problems will be gone as well.
"@cloudflare/workers-types": "^2.0.0 || ^3.0.0", | |
"react": ">=16.8", | |
"react-dom": ">=16.8" | |
"@cloudflare/workers-types": "^2.0.0 || ^3.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for that context. The desired result (no warning when using Yarn) can also be achieved by setting
"peerDependenciesMeta": {
"react": {
"optional": true
},
"react-dom": {
"optional": true
}
}
in the package.json
of remix-server-runtime
. I’ve made this change now, let me know if you disagree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's only a warning, I think we can just leave it as-is
The warning will disappear automatically once we made the switch to @remix-run/router
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this might be fixed in the future, I’d still err on the side of correctness in the meantime.
This is a warning when running yarn install
, but an error (via @yarnpkg/esbuild-plugin-pnp
) when running yarn build
:
So I think the minimum necessary change is to include react
and react-dom
as peer dependencies of remix-dev
, if nothing else. I think it’s safer to keep the peerDependenciesMeta
setting because I’m concerned there are configurations that I haven’t tested that might otherwise fail like the example above.
If you think there are alternative ways to solve the error above, please let me know and I’d be happy to adapt my proposed fix accordingly.
Closing in favour of #3594 |
Related to #683 (already closed)
react
andreact-dom
as peer dependencies for all packages that consume@remix-run/server-runtime
and don’t already havereact
(-dom
) as a dependency, because@remix-run/server-runtime
itself listsreact
(-dom
) as peer dependencies. This prevents the following warning when runningyarn install
:yarn config get @remix-run:registry
throws because the setting can’t be found