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 dependencies #246

Merged
merged 1 commit into from Mar 12, 2019

Conversation

Projects
None yet
2 participants
@sugarshin
Copy link
Contributor

commented Mar 7, 2019

Hi,

I fixed dependencies.

  • prop-types should into dependecies
  • should not @types/react-image-crop into the dependecies because not needed in production

Thank you.

@sugarshin sugarshin force-pushed the sugarshin:patch-1 branch from f59cca9 to e2dc97b Mar 7, 2019

@DominicTobias

This comment has been minimized.

Copy link
Owner

commented Mar 7, 2019

Hi thanks but prop-types is declared as a peer dependency by design - your app should install it, this avoids duplication in the final bundle.

Types are also intentionally there and do not get built into the bundle, but they are installed for people you use typescript without themselves having to manually install it. For those not using it, it’s a tiny package which does not get used so not a big deal I think?

@sugarshin

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Hi, Dominic.
Depending on the project, prop-types may not be necessary. And even small libraries as small as prop-types do not matter if they duplicate.
Including @types in dependencies is rare. And, it should be installed by the person who wants to use it. Otherwise type should be created in this repository.

@DominicTobias

This comment has been minimized.

Copy link
Owner

commented Mar 7, 2019

I don't really have an issue with moving proptypes to deps if you like, but would like to leave types there since people are depending on it being there (#200) and it wouldn't make sense for types to go missing in a patch or minor version change. Besides it is a small module which doesn't affect people who are not using it. So if you remove that change I will merge the PropType part.

@sugarshin

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

OK, I will fix it later. Thank you!

@sugarshin sugarshin force-pushed the sugarshin:patch-1 branch from e2dc97b to ec6cda4 Mar 11, 2019

@sugarshin

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@DominicTobias I fixed it.

@sugarshin sugarshin force-pushed the sugarshin:patch-1 branch from ec6cda4 to 82838a5 Mar 11, 2019

@sugarshin

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

By the way, the latest version of @types/react-image-crop is v6.0.1. You must also follow this update if you include it in the dependencies of this repository. After all, it is not a good idea. I think that I should get rid of this dependency even if I upgrade the major version.

@DominicTobias

This comment has been minimized.

Copy link
Owner

commented Mar 12, 2019

Agreed, I will remove in next major version which I'm planning to do fairly soon. I intend to change the cropping library to use pixels rather than percentages as it causes more trouble than it's worth

Will update to v6 for now

@DominicTobias DominicTobias merged commit 25e9ff8 into DominicTobias:master Mar 12, 2019

@sugarshin sugarshin deleted the sugarshin:patch-1 branch Mar 19, 2019

@DominicTobias

This comment has been minimized.

Copy link
Owner

commented Apr 16, 2019

Updated in v7 so that it is not a depedency, also PropTypes is no longer a peer dep. Note that v7 has breaking changes though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.