I personally see shrinkwrap as a step in the wrong direction, I feel like it has been explored in the past, and the general consensus is that it is something to avoid. If committing package-lock is contentious, I am okay with skipping it, but we need to be careful to lock down our own dependencies and not use wildcards.
We have flexibility in our current GitHub repos, which allow us to remix and freely link other dependent packages so we can debug right through our deps. I agree we need more stability around the release/publish steps so we can move more quickly through the collection of packages that make up a 'tools' release. @purplecabbage risingj.com On Fri, Sep 14, 2018 at 2:56 PM <raphine...@gmail.com> wrote: > No. This won't fix anything. Plus it goes directly against npm's > recommendation. Please double check the docs for the use cases of > package-lock.json vs npm-shrinkwrap.json. > > Am Fr., 14. Sep. 2018 um 23:48 Uhr schrieb Chris Brody < > chris.br...@gmail.com>: > > > A really nice alternative may be to turn the generated > > package-lock.json into npm-shrinkwrap.json (using npm shrinkwrap > > command) then commit npm-shrinkwrap.json. Then I think any other npm > > install updates would update npm-shrinkwrap.json instead of > > package-lock.json. Could be more predictable and easier to understand. > > > > This was already discussed in 2014 [1], thanks to Jesse for the link in > > [2]. > > > > Thanks for the suggestion to use npm shrinkwrap as a solution for > > cordova-cli 8.1.0 minor release in [2]. > > > > [1] > > > https://lists.apache.org/thread.html/99184622129935eb473e843e583bf6648faff279a014e8508cc2c660@1411013202@%3Cdev.cordova.apache.org%3E > > > > [2] > > > https://lists.apache.org/thread.html/f89a074add24f2ace7006b0211cf43a47cc5c1a0a65932fc22515828@%3Cdev.cordova.apache.org%3E > > On Fri, Sep 14, 2018 at 3:53 PM Chris Brody <chris.br...@gmail.com> > wrote: > > > > > > To be honest I have pretty limited experience with package lock file > > > and it is now starting to show. From Oliver's very unfortunate > > > experience I would conclude that this is something we should do very > > > carefully and not just on a whim. Some things I can think of: > > > > > > * always use recent version of npm such as npm@6.4.1 to generate or > > > update package-lock.json > > > * do not use. npm cache when generating or updating package-lock.json, > > > or use the npm cache with extreme care (also limited experience for > > > me) > > > * be extremely careful with assumptions; I think we should both > > > double-check the documentation and do our own experimentation, like I > > > did in <https://github.com/apache/cordova-cli/pull/325> to validate as > > > best we can > > > * semver package seems to be a major library package used by npm; we > > > should both read the documentation and experiment, ideally with its > > > own test cases > > > > > > On Fri, Sep 14, 2018 at 9:47 AM Oliver Salzburg > > > <oliver.salzb...@gmail.com> wrote: > > > > > > > > The problems that appear when you have linked dependencies is that > npm > > > > will pick them up as being bundled and mark them as such in the > > > > lockfile. *However* this behavior has changed in the past. At one > point > > > > this affected any direct dependency, at another point it "only" > > affected > > > > dependencies of dependencies. > > > > > > > > Either way, the result is: > > > > > > > > a) a corrupted lockfile, which has dependencies marked as bundled > > > > b) a lockfile that lists incorrect versions, resulting from linking > > > > temporary development snapshots together > > > > > > > > When you use a local npm cache and you neglected to correctly > > > > parameterize your npm calls, you now have your custom registry URL in > > > > the lockfile for every package it installed from there. This makes it > > > > unusable for others. > > > > > > > > The issue that ultimately drove us away from this concept was that > > > > locally cached packages were installed over linked modules, because > the > > > > package manager did not understand that they are linked. > > > > But because they were linked, the cached package contents were placed > > in > > > > my local development checkout of that linked module. That obviously > > > > caused all uncommitted changes to be deleted. > > > > > > > > Additionally, if you already have linked modules set up, but the > > > > lockfile says that a certain dependency is to be replaced, it will > just > > > > break your link or replace your linked code as soon as you `npm > > install`. > > > > > > > > We had so many issues with this, I'm sure I'm only remembering the > tip > > > > of the ice berg. Maybe all of this was fixed somehow, but I doubt it. > > At > > > > the time when I reported these issues, there was little interest in > > > > resolving them. > > > > > > > > However, I'm not unfamiliar with the lockfile-driven workflow as many > > > > OSS projects I contributed to use it. It is not uncommon to > completely > > > > wipe your node_modules and/or package-lock.json to rebuild it, > because > > > > of corruptions in either entity. And that is something that has been > > > > confirmed to me many times by other developers. As in "That's just > how > > > > it is." > > > > > > > > This entire area of issues was not exclusive to npm either. We > > > > extensively evaluated yarn regarding these aspects and it performed > > just > > > > as poorly. > > > > > > > > I consider these aspects unacceptable for a development workflow as > > they > > > > introduce an unreliability where I can't have one. > > > > > > > > If someone came out and told me "Hey, you've been doing it wrong all > > > > along. These are your mistakes and this is how you resolve them." > then > > > > I'd be very happy to hear that :) > > > > > > > > On 2018-09-14 15:13, raphine...@gmail.com wrote: > > > > > 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 > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org > > > > For additional commands, e-mail: dev-h...@cordova.apache.org > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org > > For additional commands, e-mail: dev-h...@cordova.apache.org > > > > >
