40818419 / react-code-input

React component for entering and validating PIN code.
MIT License
314 stars 128 forks source link

package.json misconfigured #75

Open andrewl913 opened 5 years ago

andrewl913 commented 5 years ago

Looks like react and reace-dom is not only a dependency but also a peer dependency. This can cause issues for those using mono repo workflows such as lerna or yarn workspaces..

Should remove the dependencies and place them in devDependencies

sirajalam049 commented 5 years ago

dependencies are required to run, devDependencies only to develop, e.g.: unit tests, CoffeeScript to Javascript transpilation, minification, ...

React is a dependency because it is included in the final build.

More Details: https://stackoverflow.com/a/48861985/5132337

andrewl913 commented 5 years ago

Sorry for the late reply

This is incorrect.

For reusable components:

Put a react dependency in both peerDependencies and devDependencies. devDependencies ensures React will be installed when you run npm install while developing your component, or when running tests on Travis or similar.

Putting react in dependencies will cause multiple versions of React to be installed if somebody uses your component but has a different version of React in their own package.json - having multiple versions of React not only bloats the build, but also causes errors when different versions try to interact.

https://stackoverflow.com/questions/30451556/what-is-the-correct-way-of-adding-a-dependency-to-react-in-your-package-json-for

Also imperical proof from libraries already doing this

styled-components: https://github.com/styled-components/styled-components/blob/master/packages/styled-components/package.json

material-ui: https://github.com/mui-org/material-ui/blob/master/packages/material-ui/package.json

formik: https://github.com/jaredpalmer/formik/blob/master/package.json

andrewl913 commented 5 years ago

For this reason alone we had to ditch this library and use our own.

mlunoe commented 5 years ago

I am facing the same issue. Is there an update on this?

gbhasha commented 4 years ago

me too facing same issue.

Having multiple versions of React not only bloats the build, but also causes errors when different versions try to interact. We are using latest react version (16.12.0) with hooks support, but this library causes some error due to different versions of react installed.

Kindly move the react and react-dom from dependencies to devDependencies.

To add to @andrewl913 list, another popular library which doing this.

react-dates (by airbnb) - https://github.com/airbnb/react-dates/blob/master/package.json

react-accessible-accordion - https://github.com/springload/react-accessible-accordion/blob/master/package.json

react-google-maps - https://github.com/tomchentw/react-google-maps/blob/master/package.json

react-id-swiper - https://github.com/kidjp85/react-id-swiper/blob/master/package.json

gbhasha commented 4 years ago

https://stackoverflow.com/a/48861985/5132337

This logic applies to react applications(projects). Not for reusable components.

This library is a reusable component used on other react projects. So it make sense to move the react to devDependency and inform the consumer of the library through peerDependency and avoid bloating the final build having multiple versions of react.

In the same SO thread look at another answer - https://stackoverflow.com/questions/48861868/why-react-should-usually-be-a-prod-dependency-and-not-dev-dependency/48861985#48861985

React could very well be listed as a peer dependency is some of the popuar libraries. This is because we assume that all projects that are going to use this library are going to have React installed. Like consider the case of react-bootstrap. It can only be run in react projects, so we include react as peer dependency reducing the overhead of installing it.

https://github.com/react-bootstrap/react-bootstrap/blob/master/package.json

acusti commented 3 years ago

i removed the direct react + react-dom dependency and kept them in peerDependencies in #152. it isn’t necessary to have them in devDependencies because @storybook/react takes care of installing react and building the environment on npm start already. i also fixed a handful of other issues and published the result to NPM if anyone wants to give it a try:

npm i @acusti/react-code-input
# or
yarn add @acusti/react-code-input
giannif commented 2 years ago

@acusti any chance of #152 getting merged and released?

austinbiggs commented 1 year ago

@acusti any chance of #152 getting merged and released?

Bump! Is there any chance that this will be merged?