Skip to content

@patternfly/react-console: Remove unnecessary peer dependencies#1021

Merged
tlabaj merged 1 commit intopatternfly:masterfrom
vojtechszocs:react-console-deps
Dec 12, 2018
Merged

@patternfly/react-console: Remove unnecessary peer dependencies#1021
tlabaj merged 1 commit intopatternfly:masterfrom
vojtechszocs:react-console-deps

Conversation

@vojtechszocs
Copy link
Contributor

This is a follow-up to kubevirt-web-ui-components issue 136.

Main patternfly-react package declares patternfly and react-bootstrap as regular dependencies, which pulls them into the consuming application's dependency tree.

However, @patternfly/react-console package declares patternfly and react-bootstrap as peer dependencies (in addition to having a peer dependency on patternfly-react, which in turn pulls these two specific libraries as regular dependencies).

Aside from the fact that code in @patternfly/react-console package doesn't link to neither patternfly nor react-bootstrap, this breaks the rule established by main patternfly-react package - treating those two specific libraries as regular dependencies, pulled transitively into the consuming application's dependency tree.

Having too many peer dependencies can snowball into tons of warnings in consuming libraries and/or applications. Let's try not to introduce additional peer dependencies unless we have a compelling reason to do so.

@vojtechszocs
Copy link
Contributor Author

cc @jeff-phillips-18 @mareklibra

@patternfly-build
Copy link
Collaborator

PatternFly-React preview: https://1021-pr-patternfly-react-patternfly.surge.sh

@mareklibra
Copy link
Contributor

Thanks @vojtechszocs , I'm fine with removing these peerDeps as they are provided by patternfly-react.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3493

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.131%

Totals Coverage Status
Change from base Build 3486: 0.0%
Covered Lines: 3968
Relevant Lines: 4569

💛 - Coveralls

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants