Open matthias-ccri opened 1 year ago
ggetz
commented
1 year ago 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?
matthias-ccri
commented
1 year ago Thanks @ggetz for the links! I heard there was a policy as mentioned by @gberaudo in the ol-cesium repo (https://github.com/openlayers/ol-cesium/issues/962#issuecomment-863263797) 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 (https://github.com/openlayers/ol-cesium/issues/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: https://github.com/actions/setup-node/pull/103#issuecomment-890361838
I don't want to make this comment too long, but here are some things to think about:
In your linked comment (https://github.com/CesiumGS/cesium/issues/10585#issuecomment-1217111557) 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.
You 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 dependency semver ranges to avoid incompatibilities with other package versions.
Maintainers 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.
Is 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.
ggetz
commented
1 year ago 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 ^ versions in most cases, unless there is a specific reason not to.)
matthias-ccri
commented
1 year ago 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?
Do maintainers only use npm or do some use yarn (and if so, do they use yarn 1.x or 2+)?
Because I notice Cesium has a .npmrc with package-lock=false which prevents npm from generating a package-lock.json but yarn 2+ doesn't respect that setting (and idk about yarn 1). (A core maintainer of yarn Maël Nison has strong opinions that lockfiles should always be committed, so yarn just always makes a lockfile no matter what).
I'm curious about your experience with Greenkeeper and how you say it created a burden for maintainers. What happened — I assume Greenkeeper created PRs... did it also find breakages in the builds/tests? Were those false positives, e.g. from flaky tests, or were they valid breakages? Or was Greenkeeper being a "canary" and detecting bugs in newly published 3rd party packages, that would get quickly fixed by those package authors, so it didn't really matter? In terms of noise, was it PR noise or commit noise? Was manual intervention needed for this noise, like merging PRs that Greenkeeper opened? I'm curious because if Greenkeeper is used with lockfiles, then I think it should only detect breakages from bleeding edge packages. So essentially it should: 1) refresh the lockfile, 2) run builds and tests, 3) on failure, open an Issue or PR with the failing build. 4) On success it could either do nothing, or commit an updated lockfile (though this is commit log noise, so you could skip this. Arguably if everything is working then it doesn't matter which exact deps maintainers use in development. But if it does matter, then make greenkeeper commit the lockfile periodically.)
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:
package.json dep changes (like a new dep added/removed). So a contributor's PR should only have lockfile changes if they've changed package.json. If there is unintentional lockfile diff, typically this mistake happens because of a misconfigured npmrc, like pointing to a different registry url and/or the contributor deleted the lockfile and reran npm install. It's a simple fix to revert the change: git checkout - package-lock.json.
Note: It's possible for a PR author to intentionally update/refresh their lockfile, but then they probably know what they're doing so they can deal with the merge conflict risk.package.json itself is valid / no merge conflicts). So you'd just run npm install and the lockfile is good again. Mark it as resolved and move on to the next conflict.main changes, normally git would allow merging the PR if there are no merge conflicts. But if both main and the PR have lockfile changes, this would create a merge conflict blocking the PR from merging, and the PR author would need to locally resolve the conflict. To scope this risk: keep in mind that 1) it's usually rare for a PR to have lockfile changes, and ) if there are 2 or more merge conflicts the developer would have to resolve locally anyway.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
gberaudo
commented
1 year ago 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. Instead, we prefer renovate (see instructions. It allows to write custom rules for grouping updates in 1 PR and that makes all the difference. Typically all patch updates are probably going to pass, so having them all in one PR saves a lot of time. Actually I group minor + patch together, but you see the idea.
ggetz
commented
1 year ago 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.jsonin 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.