Thanks for picking this up again Chris. I think now is a better time for
second thoughts than later.

I've had a look at your experiment and the behavior you observed is to be
expected and desirable, as I explained in more detail in a comment [1]
there. As I also mentioned there, deleting and regenerating package-lock.json
is a valid approach _if and when_ you want to do a full dependency update,
as we regularly do.

Also, thanks for posting Oliver's message here for better visibility in
this discussion. What I _do_ find a bit disturbing is the problems he
mentioned regarding linking (as in `npm link`) of different modules which
are all using lock files. He expressed his concern regarding that
particular use-case again in a comment [2] of a PR where we touched that
topic. I think it is important we test this, since the ability to link
modules is vital for our development workflow and I have no experience with
package-lock.json in projects where a lot of linking is necessary.

Finally, I think we might need to re-evaluate our presumed knowledge about
the topic at hand. I encourage all those interested to read [3][4][5] so we
all know what we are talking about. I had my facts wrong too and nobody
corrected me, when I uttered them here in this thread. So here's a quick
(probably incomplete) round up of what package-lock.json does and does not
do:

   - It does provide a snapshot of the dependency tree that can be
   committed into source control to avoid automatic updates of (transitive)
   dependencies break the build _during development and CI_
   - It _does not_ ensure that a user installing the published package gets
   exactly the dependency versions that are specified in package-lock.json.
   That is what npm-shrinkwrap.json [5] is for.
   - It does speed up installation of dependencies in conjunction with `npm
   ci` by skipping the entire dependency resolution and using the versions
   from the lock file.
   - It is required to be present for `npm audit` [6], although it could be
   generated ad-hoc.
   - It is possible to manually tinker with the lock file to fix audit
   issues with transitive dependencies that have no update available. This
   requires some special care to prevent npm from resetting these manual
   changes, but it's a valuable last-resort option. However, this is far more
   useful with npm-shrinkwrap.json to create releases without security
   issues.

With that cleared up, my stance on committing package-lock.json is as
follows:

   - Faster CI installations and faster/easier usage of `npm audit` are
   purely convenience features for me.
   - Consistent developer builds and updates only on-demand are real
   advantages for me. I just recently spent hours finding out why some tests
   of cordova-lib were suddenly failing. It turned out it was caused by an
   update to `jasmine@3.2.0`.
   - If the package-lock.json really should interfere with our ability to
   link repositories for development, then that would be a deal breaker for me.

However, the primary goal that I wanted to achieve was _immutable
releases_. That is, installing e.g. `cordova@9` should _always install the
exact same set of packages_. What we need for that is npm-shrinkwrap.json.
So IMO whether we decide for or against using package-lock.json, we should
lock down the dependencies for releases of our CLIs, platforms and possibly
plugins by generating and committing a npm-shrinkwrap.json to the _release
branch_ before packaging the release.

Cheers,
Raphael

[1]: https://github.com/apache/cordova-cli/pull/325#issuecomment-421336025
[2]:
https://github.com/raphinesse/cordova-common/pull/1#issuecomment-420950433
[3]: https://docs.npmjs.com/files/package-locks
[4]: https://docs.npmjs.com/files/package-lock.json
[5]: https://docs.npmjs.com/files/shrinkwrap.json
[6]: https://docs.npmjs.com/getting-started/running-a-security-audit

Reply via email to