-
Notifications
You must be signed in to change notification settings - Fork 185
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
kie-issues#1365: Revisit all React-dependent packages on kie-tools
declaring react
as a dependency
instead of a peerDependency
#2559
kie-issues#1365: Revisit all React-dependent packages on kie-tools
declaring react
as a dependency
instead of a peerDependency
#2559
Conversation
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.
Left a question inline... Thanks for the new syncpack setup, looks great!
…isit-all-React-dependent-packages-on-kie-tools-declaring-react-as-a-dependency-instead-of-a-peerDependency
"react": "^17.0.2", | ||
"react-dom": "^17.0.2" |
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.
[curious-question] Shouldn't we move this react
and react-dom
to the devDependencies
object so we can still work with this editor in dev mode without accounting for another package in this workspace to bring them? 🤔
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.
@lordrip Shouldn't be the final package, importing @kie-tools-examples/base64png-editor
, to provide react in dependencies
of devDependencies
?
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.
@fantonangeli , yes, the final package (consumer) should definitively provide react. I was referring to another case though.
Let's ignore for a moment that @kie-tools-examples/base64png-editor
belongs to this repository, so the package json for it would look like the following:
{
"name": "@kie-tools-examples/base64png-editor",
...
"dependencies": {},
"devDependencies": {
"typescript": "5.0.0"
},
"peerDependencies": {
"react": ">=17.0.2 <19.0.0",
"react-dom": ">=17.0.2 <19.0.0"
}
}
Now, performing a yarn install
won't bring react
to the project, so the project will fail when trying to run it in standalone mode as there's no react dependency in the node_modules
.
Now, this repository is slightly different as it's very likely that some other package is bringing react
to the node_modules
, hence making it work, but if by any chance I bootstrap just the @kie-tools-examples/base64png-editor
maybe it will fail, as some dependencies of other packages won't be brought?
I'm not familiar with how pnpm
works in terms of bootstraping packages and whether if it will download dependencies for unresolved packages, but if it does, then I think we're good, otherwise, 💥
Maybe 💥 is a bit too dramatic
Closes apache/incubator-kie-issues#1365
Description:
React libraries shouldn't force a specific version of React, but rather declare its compatibility with a React version. This is most likely causing problems for people depending on React libraries/components published by kie-tools.
We need to check if Syncpack works for peerDependencies too, as we need alignment between all packages.
Notes:
kie-tools
packages to use inbackstage-plugins
Here is an analysis of the packages using react and which packages have been updated:
https://docs.google.com/spreadsheets/d/171HP_YReV1PSke71WE3gdxyrU8m4heTCpMKlEdMy-l0