mgol / check-dependencies

Checks if currently installed npm dependencies are installed in the exact same versions that are specified in package.json
MIT License
115 stars 30 forks source link

Suggestion - suggest npm ci instead of npm install #37

Open jackgeek opened 5 years ago

jackgeek commented 5 years ago

For teams using package.lock.json advising the user to use npm install is not good advice. WOuld it be possible to add configuration to allow it do advise running npm ci instead?

mgol commented 5 years ago

As far as I understand, npm ci is meant for CI systems, not for running on a local machine, therefore npm install is still applicable. npm ci is also mainly meant to be run on a state with no node_modules present (if the folder is present, it gets deleted first) so a dependency check before running npm ci doesn't make much sense to me - on CI it should be run always, locally - it shouldn't.

jackgeek commented 5 years ago

@mgol Thanks for your quick response.

This is not quite true. We use npm ci to ensure that the packages we install comply with package.lock.json. And we do this locally. The impact of running npm install is that it will change your package.lock.json and therefore you do not get the benefit of its protection.

We advise all our developers to run npm ci instead of npm install for this reason.

jackgeek commented 5 years ago

BTW happy to submit a PR for this if you agree with it

mgol commented 5 years ago

I'm fine with this being an option, I think npm install should still be the default. Perhaps a new packageInstallCommand being "install" by default would solve the issue? You'd be able to define it to be "ci" for your purposes; it'd also make it easier to support package managers that use a different command than install (if there are any).

PRs are welcome!

jackgeek commented 5 years ago

@mgol great, please expect a PR forthwith.

May I suggest that the packageInstallCommand default to "npm install" then you could have the option of changing it to "yarn install" or "pnmp install" or one of the many other npm alternatives.

mgol commented 5 years ago

@jackgeek There's already the packageManager option that controls which package manager to use. This is needed because some of them use a different folder for their dependencies (e.g. Bower uses bower_components).

BTW, currently only npm & bower are officially supported (see the README). I think adding support for more should be doable, in many places there's logic that checks whether packageManager === 'npm' and if not, assumes it's Bower. If that logic was reversed, more package managers could be supported as most behave like npm. PRs for that are also welcome if you're interested (please remember about tests!).

jackgeek commented 5 years ago

Hi @mgol, This is taking me longer than I anticipated due to eslint and formatting rules. Do you use an auto formatter? Is there one I can use to get the style for your project? If not have you considered using prettier? This is what I use for my projects and it is excellent.

mgol commented 5 years ago

Yeah, I’d use Prettier if was setting up the repo now. I’m fine with a PR that introduces it but I think you’d first need a PR to my eslint-config-mgol to remove all formatting rules from the config. I have a check there that all defined rules are either explicitly enabled or disabled so those rules need to be set to disabled.

mgol commented 4 years ago

@jackgeek I went ahead & updated eslint-config-mgol & introduced Prettier to check-dependencies. Hopefully the setup is now more to your liking. :)