+1 Thank you for the clarity! I am completely supportive of this. @purplecabbage risingj.com
On Fri, Sep 14, 2018 at 3:28 PM <raphine...@gmail.com> wrote: > Please note that I'm not suggesting to commit shrinkwrap to the master of > every repository. That is what had been discussed in the mailing list > thread from 2014 that you shared. That option was dismissed then, and > rightly so. It is also strongly advised against in the npm docs. > > I suggest using it only in release branches of CLI or other non-library > releases, so they are immutable and don't mutate over time. _This is > exactly the intended use_. And it comes at no cost other than having to run > `npm shrinkwrap` right before the release. > > Plus, it allows us to override transitive dependency versions for a release > if absolutely necessary. > > Am Sa., 15. Sep. 2018 um 00:16 Uhr schrieb Jesse <purplecabb...@gmail.com > >: > > > 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 > > > > > > > > > > > > > >
