Raphael I did see your argument for considering shrinkwrap once before in [1] (seems to be buried at the end of another discussion). But then I saw the point *not* to turn package-lock.json into npm shrinkwrap in [2] and maybe in [3]. So I was starting to get a bit lost here.
It would be great if you would have a chance to explain your proposal in more detail, maybe walk us through more slowly. Sorry to be a pain here. [1]: https://lists.apache.org/thread.html/c06d35b6944fb44a04449a7baf10797f8ae11cbcfddb3e90851dc0b9@%3Cdev.cordova.apache.org%3E [2]: https://lists.apache.org/thread.html/463315d120fae1ed78aa825f58b41b2cce0397dba58dfdfecc442d91@%3Cdev.cordova.apache.org%3E [3]: https://lists.apache.org/thread.html/e336ac350b6208e6ff3e3b852d0be9f7cb88dceb522b2fe0390582bb@%3Cdev.cordova.apache.org%3E On Mon, Sep 17, 2018 at 7:31 AM <[email protected]> wrote: > > I do not agree with your understanding here. We should use shrinkwrap for > all releases. Please see my _very extensive_ argument and explanation on > that topic in the thread you referenced [1]. > > It has all kinds of references too, some of which you then found again > yourself in a later message of the same thread [2]. I have to ask: did you > read my message [1] at all? > > [1]: > https://lists.apache.org/thread.html/c06d35b6944fb44a04449a7baf10797f8ae11cbcfddb3e90851dc0b9@%3Cdev.cordova.apache.org%3E > [2]: > https://lists.apache.org/thread.html/bce77e5892543003c3b3671cb6a2e9e02c33485073e691c6cf2c5fcf@%3Cdev.cordova.apache.org%3E > > Am Mo., 17. Sep. 2018 um 13:14 Uhr schrieb Chris Brody < > [email protected]>: > > > Originated in another thread ([1]), moving into a new thread. > > > > My understanding is that we should only do npm shrinkwrap if needed to > > deal with a dependency problem in an end-user package such as > > cordova-cli. > > > > I would like to ask people to please use references whenever possible. > > > > [1] > > https://lists.apache.org/thread.html/7f92561d382f143aaf49e083bbe215dcf95a3f4d8b6e3cbb6089a5f3@%3Cdev.cordova.apache.org%3E > > > > On Mon, Sep 17, 2018 at 5:38 AM Oliver Salzburg > > <[email protected]> wrote: > > > > > > +1 to shrinkwrap before publish > > > > > > On 2018-09-15 00:27, [email protected] 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 < > > [email protected]>: > > > > > > > >> 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 <[email protected]> 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 < > > > >>> [email protected]>: > > > >>> > > > >>>> 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 <[email protected]> > > > >>> 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 [email protected] 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 > > > >>>>> <[email protected]> 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, [email protected] 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 `[email protected]`. > > > >>>>>>> - 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: [email protected] > > > >>>>>> For additional commands, e-mail: [email protected] > > > >>>>>> > > > >>>> > > --------------------------------------------------------------------- > > > >>>> To unsubscribe, e-mail: [email protected] > > > >>>> For additional commands, e-mail: [email protected] > > > >>>> > > > >>>> > > > > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: [email protected] > > > For additional commands, e-mail: [email protected] > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [email protected] > > For additional commands, e-mail: [email protected] > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
