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]
