Skip to content
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(components): remove @carbon/icons from peerDependencies #5656

Merged
merged 4 commits into from
Mar 20, 2020

Conversation

joshblack
Copy link
Contributor

We currently don't' use @carbon/icons in our Vanilla JS and this will prompt folks to install an additional dependency even if they won't use it directly, or if code they use from the library does not use it. Let me know if I'm missing a place where it's being used!

For reference, without this change folks would technically need to install the following packages to remove warnings:

npm install carbon-components @carbon/icons carbon-components-react carbon-icons

Changelog

New

Changed

  • Update package.json for components to not have peerDependencies

Removed

@joshblack joshblack requested a review from a team as a code owner March 18, 2020 23:10
@ghost ghost requested review from aledavila and emyarod March 18, 2020 23:11
@netlify
Copy link

netlify bot commented Mar 18, 2020

Deploy preview for carbon-components-react ready!

Built with commit 57a5972

https://deploy-preview-5656--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Mar 18, 2020

Deploy preview for carbon-elements ready!

Built with commit 57a5972

https://deploy-preview-5656--carbon-elements.netlify.com

@asudoh
Copy link
Contributor

asudoh commented Mar 19, 2020

Vanilla users need to write the HTML by their own. That said, how can vanilla users get the SVG contents without @carbon/icons?

@joshblack
Copy link
Contributor Author

@asudoh I believe the dependency is still valid/needed to install, just that it doesn't need to be listed as a peer dependency 👍

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me

@asudoh
Copy link
Contributor

asudoh commented Mar 19, 2020

@joshblack Did you want to add @carbon/icons to dependencies you mean...?

@joshblack
Copy link
Contributor Author

@asudoh I think if a team would like to use the package they can install it, but there should no need to rely on carbon-components listing it as a dependency or peer dependency if the library itself does not use it.

Listing it as a peer dependency would lead to the warning in projects even if they don't need or want that dependency, and would be frustrating given that the library itself doesn't use it. Listing it as a dependency would mean that folks are consuming the dependency through hoisted node_modules which would lead to unintended behavior.

If the library is using it in code, we 100% can bring it in but it seemed like that was not the case.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Signing off based on @joshblack's latest response, though in most of the use cases of vanilla library application code needs @carbon/icons (or copy contents from it) to create its markup.

@joshblack joshblack merged commit 212d4b0 into master Mar 20, 2020
@joshblack joshblack deleted the joshblack-patch-2 branch March 20, 2020 16:36
renmaddox added a commit to renmaddox/carbon that referenced this pull request Mar 20, 2020
renmaddox added a commit to renmaddox/carbon that referenced this pull request Mar 20, 2020
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.

4 participants