finos / openfin-react-hooks

A collection of React Hooks built on top of the Openfin API - from Scott Logic
Apache License 2.0
48 stars 19 forks source link

Shouldn't react-scripts be a devDependency ? #17

Closed maoo closed 4 years ago

maoo commented 4 years ago

I was scanning (runtime) dependency licenses being pulled by the project at build time, using the following commands:

npm i -g node-license-validator
git clone git@github.com:ScottLogic/openfin-react-hooks.git
cd openfin-react-hooks
npm i
npm run compile
cd demo
npm i --prod
node-license-validator . --allow-licenses MIT Apache-2.0 BSD BSD-3-Clause

The result I get is a long list of unidenfied/unfriendly licenses that are pulled from the dependency react-scripts, which seems to be more suited as devDependency rather than a (runtime) dependency.

I locally updated demo/package.json accordingly and run the scripts, everything seems to be working as expected; I also added license: "Apache-2.0" into the same file, otherwise node-license-validator would complain about it.

Would be great to add these license validation steps as part of the CircleCI build process.

This work is part of the contribution process to FINOS, see https://finosfoundation.atlassian.net/browse/CONTRIB-56

Thanks!

oriondean commented 4 years ago

Hi @maoo, thanks for getting in touch!

I agree that react-scripts should be listed as a devDependency rather than a dependency.

The reason why it's currently listed as a dependency is due to https://github.com/facebook/create-react-app/issues/2696. create-react-app out of the box puts react-scripts as a dependency rather than devDependency for compatbility purposes. There's nothing stopping us in our particular use-case setting it as a devDependency so we should be fine to swap it over to the more appropriate place.

maoo commented 4 years ago

Thanks @oriondean !

Do you see any problems adding/updating all the package.json files to use Apache-2.0 as license?

oriondean commented 4 years ago

Nope! Sorry I didn't get back to you, been busy travelling. @ColinEberhardt has stepped in during my absence to stick a PR up (thanks!)

ColinEberhardt commented 4 years ago

This should all be resolved now.