-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(types): Set @types/react as optional peer dependency #5542
fix(types): Set @types/react as optional peer dependency #5542
Conversation
I'm not sure that peer deps are the correct place for the types? only this package needs this range of types. the duplication in the issue is coming from npm/yarn not being able to reconcile and dedupe a common version. It does seem to break which isn't great but alternatively if it's a peer dep the user would still have an invalid setup trying to use the v17 types, AND npm users would get warnings saying they are missing a peer dep because peer deps meta isn't supported in npm (maybe in v7?). not sure what the correct thing to do is... |
About npm v7: It does installs peer deps by default. Npm v6 does not however. |
Peer deps meta should be supported as of 6.11.0: This release is a lil over a year old now, so most people should be good with the peer deps meta support. Warnings might be ok otherwise, since they'll likely be seeing warnings about the jquery peer deps in bootstrap already :P I did check the build against the react 17 types and didn't see any issues. I think it should be ok for the types to support >=16.9.35? |
Thanks @kyletsang for taking care of this. It would be super helpful to get a release with this fix once you decided on the final solution as this breaks compilation for projects trying to use react 17 types. |
@ceisele-r it will continue to be broken with react-17 types until RB supports that version...moving it to a peer dep will only move the conflict |
package.json
Outdated
@@ -139,9 +139,15 @@ | |||
"webpack": "^4.44.2" | |||
}, | |||
"peerDependencies": { | |||
"@types/react": ">=16.9.35", |
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.
I think the initial step is just to change the normal dep to >=
range to allow the 17 types and enable deduping. That is if we are actually compatible with those types
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.
Is there a way for us to test this locally to make sure dedupe works as expected?
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.
no, but it should, has up until now at least
c95d3b7
to
757f452
Compare
Fixes #5541