New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(Codebase) Commit package-lock.json for stability #11242
Comments
Hi @matthias-ccri, thanks for feedback! Historically, CesiumJS has intentionally avoided committing lock files. Note that committers do remove their lockfile periodically as part of the release process. I'm curious to know if you've run into this in practice while doing development? |
Thanks @ggetz for the links! I heard there was a policy as mentioned by @gberaudo in the ol-cesium repo (openlayers/ol-cesium#962 (comment)) but was unable to find it, so thank you. Anecdotally, yes in my experience lockfiles provide stability which is indispensable. The aforementioned ol-cesium issue (openlayers/ol-cesium#962) happened because there was no lockfile. The result is that I was unable to pull the repo and build it, because the referenced dependencies were newer (and now the build was broken). For context, this is the package.json at that time: no dependencies, only peer and dev deps with standard caret ranges, normal stuff. I am experienced with dependency management, package managers, CI, etc, and I'm firmly convinced that there is no valid reason to gitignore lockfiles, even for libraries, and that actually you're harming the codebase in a slight but critical way. In my experience, I've seen the pros and cons of lockfiles and the pro (stability) far outweighs the cons. In fact I've make a complete list of the cons (or counterarguments) in the SO answer that I authored so I do think that I grasp both sides of the argument. I wish I had more public examples of issues I've encountered with missing lockfiles. This comment is pertinent though: actions/setup-node#103 (comment) I don't want to make this comment too long, but here are some things to think about: In your linked comment (#10585 (comment)) you reference the Twilio blog post as if it is the authoritative voice on the recommended best practice regarding lockfiles. But they are one voice out of many, and there are major opposing voices:
You also mentioned pinning deps and maintainers clearing the lockfile when releasing. The point I want to underscore here is that both of these things are good, and they don't affect the issue of committing lockfiles. pinning depsYou can still pin deps with or without a committed lockfile. Committing lockfiles has no bearing on pinning deps or how you would set your package's clearing the lockfileMaintainers can still clear their lockfiles, and it is good for libraries to do this often, and the fact that the lockfile is checked into git does not prevent this, it only enhances it with a paper trail of what changed. The only downside is that it's marginally inconvenient to deal with lockfile diffs and noise, but that's a small price to pay for stability. Again, I'd refer you to the package managers (npm, yarn, pnpm) that all say "commit the file". There are ways of mitigating the diff noise (marking the file as binary), and package managers automatically resolve merge conflicts now. about CesiumIs your main objection based on the idea that since Cesium is a library, its maintainers should detect incompatibilities as soon as possible, and the way to do that is by installing the most bleeding edge npm packages every time? Because new installs will get the bleeding edge npm packages, and so the Cesium project wants to run its builds and tests against the bleeding edge set of deps to serve new consumers. This seems like the reasoning behind why they say "no lockfile for libraries". My response to this is basically that all of this can still be achieved with lockfiles.
Note that gitignoring the lockfile only serves the "bleeding edge"/"new consumer" case, not the "existing consumer" case where an existing consumer already has an app with Cesium installed, with a lockfile and a certain set of other deps (outdated deps to some degree). Without lockfiles, there is no way to even think about investigating non-bleeding-edge package incompatibilities. And a bleeding edge case becomes a non-bleeding-edge case as soon as the public npm package ecosystem changes. There is no way to capture a "known breaking example" in a git branch. It's utter chaos, once you realize what you're losing out on by not tracking lockfiles in git. To conclude, I really believe there is no valid reason for even a library to not use and commit lockfiles. I'd like to hear your thoughts in response to the points I've made. I'm opening this topic because I respect the cesium project and I really want it to be stable for new collaborators like me when I pull changes from the repo; I don't want to spend time debugging build breakages that are entirely preventable by using lockfiles. Thanks again, I look forward to your thoughts. |
Thanks for the thorough analysis @matthias-ccri. Yes, the main reasons on why CesiumJS did not commit lockfiles were those related to libraries vs apps. I think you provide some valid reasoning here as to why lockfiles can be beneficial to libraries. In practice for development, surprise breaking don't come up too often, and we are a bit cautious about the burden merge conflicts may end up being. I'd be interested to hear what other contributors run into in terms of surprise breakages and if it's a common blocker for contributions. If we do move to using lockfiles, we'll want to ensure we have our workflow properly document and we have any relevant tooling in place. (We did use Greenkeeper previously, and found that it was generating a lot of noise and overhead for our developers to keep up with. We've since loosened our package versions and are using |
Thanks for the reply @ggetz , It sounds like you are open to the change, but I understand it's a process change that requires investigation, coordination and documentation. Ultimately the choice is up to you and other Cesium maintainers, but I'm happy to bring up the issue and I'm interested in seeing the outcome. Could you clarify a few things for me about the current situation?
A quick note on merge conflicts. It's true that lockfile changes will generate merge conflicts. This might cause friction for the PR merging workflow but it depends. Here are some points:
Alright so I think this topic is in a good place so I'll let you and the Cesium team handle it from here, but let me know if I can assist with anything. Thanks again |
Hi @ggetz, here is the ol-cesium / Camptcamp experience. For ol-cesium we finally decided to commit the package-lock.json file. The reason is that merge conflicts are reare and, when they occur, easily fixable by removing the lock and regenerating it. In addition, it plays well with dependency updaters and audit tools. We had a bad experience with dependabots because it opens 1 PR per dependency and this quickly becomes unmanagable. |
Awesome, thanks for your input @gberaudo. We'll be sure to checkout renovate based on your recommendation. |
Hello!
I am wondering if you can commit a working package-lock.json to the repo. Committing lockfiles to git is a best practice and the fact that there is no
package-lock.json
in the repo means that it might be broken for me when I check it out.The problem is that the "latest versions" of npm packages change over time, so a semver range like
"pkg": "^8.0.0"
will resolve to different versions over time, i.e. instability.Cesium maintainers may have made the decision to gitignore the lockfile because of reasoning like "it's a package so it should always get the latest versions of deps". However, this thinking is seriously flawed because 1) developer machines don't get the latest deps because they still have a lockfile (it's just not checked into git so actually everyone has a different lockfile), and 2) checking in the lockfile still allows you to change it and regenerate it, you just get to track those changes in git, making them visible and shared so everyone has the same working codebase.
Again, it is a standard practice and a best practice to commit the lockfile. See the best practice link for the consensus and reasoning, and more reasons why even a library needs a lockfile.
The text was updated successfully, but these errors were encountered: