-
Notifications
You must be signed in to change notification settings - Fork 5.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(cli): adds missing dependencies #24699
base: master
Are you sure you want to change the base?
Conversation
Changed Packages
|
@@ -38,7 +38,9 @@ | |||
"react-use": "{{versionQuery 'react-use' '17.2.4'}}" | |||
}, | |||
"peerDependencies": { | |||
"react": "{{versionQuery 'react' '^16.13.1 || ^17.0.0 || ^18.0.0'}}" | |||
"react": "{{versionQuery 'react' '^16.13.1 || ^17.0.0 || ^18.0.0'}}", | |||
"react-dom": "{{versionQuery 'react-dom' '^16.13.1 || ^17.0.0 || ^18.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.
Hmm, not sure we necessarily want these as peers deps, we don't use them directly right? any strong opinion here?
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.
@Rugvip, after reading your comment, I went and took a second look at the error message. Actually, the problem starts from /community-plugins/workspaces/<plugin-name-here>/node_modules/@backstage/app-defaults/dist' node_modules/@backstage/app-defaults/dist/index.esm.js
. So, after inspecting the @backstage/app-defaults
package.json I've found that this package lists these very same peerDependencies in its own package.json. Now it is clear to me that adding the peerDependencies again is irrelevant. A better question IMO is: What package should be tasked with installing the required peerDeps that @backstage/app-defaults needs? Is it the brand-new frontend plugin or should it be performed at a higher hierarchy in the repo? WDYT?
In the meantime, I'll remove the peerDependencies but I'll leave only the devDependencies section. Please let me know if that's an acceptable solution 🙏
fix(cli): adds missing dependencies Addresses: - !24674 Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com>
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
Hey, I just made a Pull Request!
✔️ Checklist
Signed-off-by
line in the message. (more info)