-
-
Notifications
You must be signed in to change notification settings - Fork 67
fix: Loosen dependency requirements #182
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
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔇 Additional comments (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #182 +/- ##
=======================================
Coverage 95.84% 95.84%
=======================================
Files 23 23
Lines 842 842
=======================================
Hits 807 807
Misses 35 35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We generally use exact version references to allow for deterministic testing. The solution is to continuously update the dependencies. |
|
The label |
|
Seems like something that a lockfile could help with. |
|
There is a package-lock file. If you look at merged PRs, you can see that they are opened automatically. If there are dependencies that are not updated, then it may be a bot issue we'd need to look into. |
|
Therefore you already have everything to make your tests deterministic and there's no point in carrying on your shoulders the burden of responsibility for updating consumer's dependencies! :) |
|
There are several reasons why it is the way it is. One being that package-lock can get corrupted and may need to be recreated, in which case the versions would be all over the place if we didn't fix them in the package.json as well. But this is beside your original point: dependencies should be automatically upgraded by dependabot, so if a dependency is not upgraded, please let me know which one it is. |
|
The lock file is stored in git. There is no way that it can become corrupted that we can't just revert to a working version. And even if we did need to start over for some reason, we could do so while keeping the versions the same - it's a bit complicated to do so, but straightforward once you understand how. If every project were to pin dependencies you would end up with a dozen copies of popular dependencies because it prevents them from being deduplicated when slightly different versions are used. |
|
First, thanks everyone for the thoughtful discussion. Over the years we’ve run into just about every issue imaginable around package-lock.json corruption and edge cases. That’s one reason why we keep the lockfile committed and rely on locked dependency versions for reproducible builds. It would be inconsistent to say “we have a lockfile to lock versions” while also expecting package.json to remain version-unlocked. Our intent is to lock versions. As a final note on the topic, we discourage changing dependency versions as our CI tests only run against the pinned versions. If you’d like to try different versions and run your own CI, please feel free to fork the repo, remove the lockfile, and rebuild, considering the risks. |
Just so we're in the clear: no one's advocating against the presence of the lockfile, which definitely should stay! We're advocating against pinning dependency versions for end consumers, which pushes your determinism problem onto all consumers and often causes duplicate trees and missed/delayed security/bugfix releases. |
|
It’s a trade-off. We prefer pinning direct dependencies because it guarantees our CI runs against the exact versions we ship. In the wild, upstreams occasionally don’t follow semver, and unpinned ranges can let those breakages reach developers before we can catch them. With pins, our CI identifies regressions without passing risk downstream. Sure there's the downside of duplicate installs, but that’s mostly bloat rather than a functional issue, and developers who want a single version can use npm overrides. In our view, pinning defaults to stability while still giving a conscious escape hatch. |
|
I will reformat the title to use the proper commit message syntax. |
|
@mtrezza, FYI, since this PR has not been merged, @parse/node-apn is currently a subject to GHSA-65ch-62r8-g69g, GHSA-554w-wpv2-vw27, and GHSA-5gfm-wpxj-wjgq. I have rebased the PR to fix conflicts and bumped node-forge while at it. |
Using exact dependency versions can be harmful, as it blocks downstream bug fixes, security patches, and new features. It can also increase the risk of duplicate packages in node_modules, leading to subtle, hard-to-debug issues. This PR changes all user-facing dependencies from x.y.z to ^x.y.z, allowing end users to automatically benefit from compatible updates as they are released.
b9fd03c to
f6416b6
Compare
|
@wojtekmaj How is the node-forge fix related to introducing version ranges? Isn't a node-forge dependency bump all that's needed for the fix? |
|
It is, but it requires an explicit action on your side - this time and, crucially, every time it happens ever again. At the moment, @parse/node-apn |
|
@wojtekmaj If a security vulnerability was only fixed in the next major version of a dependency, then your suggested caret range wouldn't be enough. To follow through with your argument we'd have to set all dependency versions to wildcard range, which means no robustness. This confirms that the solution is an explicit dependency upgrade. We'll therefore keep the exact versions in package.json, and also for the aforementioned reasons. To fix the node-forge vulnerability you could open a PR which upgrades the dependency. I will investigate why the PR hasn't been opened automatically, which should have happened. |
It was not. It was a bugfix. |
|
It turns out that the PR was automatically opened in #185 yesterday. The reason this wasn't opened earlier was that there was no fix available. This confirms that your automated dependency upgrade process to remediate vulnerabilities works. |
|
It doesn't. Dependabot raised a PR for you, but ultimately, the whole ecosystem waits for you to take action. |
|
@wojtekmaj That is our policy across the organization. Please refer to my previous comments for the reasons why. |
|
@mtrezza Any ETA on when you'll push a new release? |
Using exact dependency versions can be harmful, as it blocks downstream bug fixes, security patches, and new features. It can also increase the risk of duplicate packages in node_modules, leading to subtle, hard-to-debug issues.
This PR changes all user-facing dependencies from x.y.z to ^x.y.z, allowing end users to automatically benefit from compatible updates as they are released.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.