openfisca / legislation-explorer

Explore legislation formulas and parameters.
https://legislation.demo.openfisca.org
GNU Affero General Public License v3.0
26 stars 12 forks source link

Specify the minimum supported version of npm #153

Closed bonjourmauko closed 6 years ago

bonjourmauko commented 6 years ago

Based on #145 (package-lock.json), I think it is important to specify the minimum version of npm required in the package.json file (v5).

bonjourmauko commented 6 years ago

@MattiSG Actually, the objective was not to add a contraint to node version but to nvm's npm's.

Why? Because of the introduction of a package-lock.json file in #135. If we do not communicate to people that at least nvm@5 npm@5 is required, the said file is just decorative and prone to confusion, as it has no enforcing value.

I did initially think nvm@5 npm@5 was not compatible with node@6, but that's not necessarily the case. Changed description and commit message to reflect this.

MattiSG commented 6 years ago

not to add a contraint to node version but to nvm's.

I'll assume for the rest of the discussion that you meant npm (Node Package Manager) and not nvm (Node Version Manager) 🙂

~If that is indeed the aim, then why add a constraint on the Node engine? Node engine constraints are a nightmare in production, as the engine is most of the time installed at machine level.~ → Ok, this was changed in a more recent commit.

If we do not communicate to people that at least nvm@5 is required, the said file is just decorative and prone to confusion, as it has no enforcing value.

I disagree. If there is a package-lock file and the current NPM version ignores it, then it is just ignored. Actually, if you npm install, it will get updated and does not have more “enforcing value” than not being here. The main aim is to provide a snapshot so that if things break, we know what was the exact dependency status.

engines is here to “specify which versions of npm are capable of properly installing your program” (doc). I am perfectly able to install and run legislation-explorer with npm@3. If you add npm>=5 in engines, then you are telling me the software won't run, which transitively means that I will have to update my global npm version and possibly my Node engine…

Don't get me wrong: I'm all for adding and maintaining and engines field. I just think that if it is here, it should accurately expose versions for which that software does or does not work, not versions for which some additional feature is made available. If you really feel strongly about locking dependencies, then let's just add a shrinkwrap file and made that npm@3, which has a much wider installed base than npm@5.

bonjourmauko commented 6 years ago

Thanks @MattiSG

I disagree. If there is a package-lock file and the current NPM version ignores it, then it is just ignored. Actually, if you npm install, it will get updated and does not have more “enforcing value” than not being here. The main aim is to provide a snapshot so that if things break, we know what was the exact dependency status.

If the file is not taken into account in production, then we won't have the snapshot so that if things break, we know what was the exact dependency status, and so it becomes purely decorative.

If it is, then we need to tell users under which conditions it works in production, so we can reproduce any problem the may have in their local machines.

Unless npm install --production takes into account the package-lock.json file, that won't be the case, hence being more a confusion than a clarity factor.

I think we want to help people to contribute, not prevent them to do so. I'm ok in removing the engines field, as long as we make explicit which version is running in production.

MattiSG commented 6 years ago

If the file is not taken into account in production

I think you're confusing npm-shrinkwrap and package-lock use cases. Please read through the doc link I gave earlier. If you really want to add production replicability we need to maintain a shrinkwrap. That also probably entails maintaining version numbers and adding CI to check for its up-to-dateness.

I'm happy that we go this way, but in such a case this PR needs to at least:

  1. Turn package-lock into npm-shrinkwrap.
  2. Add CI for checking that each package.json change also impacts the shrinkwrap file.

In my view it should also, as it aims at making the promise of fully replicable installs:

  1. Detect the first versions of Node and NPM for which this software does not work and define them in the engines field.
  2. Define a versioning strategy.
bonjourmauko commented 6 years ago

Thanks for the links provided. I'll be closing for now, as the production case wasn't effectively in the scope of your original proposition.