dependabot / feedback

The old feedback repository for Dependabot. Click below for the new repository.
https://github.com/dependabot/dependabot-core
93 stars 28 forks source link

Javascript Dependencies: Update package.json minimum version for in-range updates. #91

Closed rwjblue closed 5 years ago

rwjblue commented 6 years ago

When a dependency of a javascript pacakge with a lock file is released dependabot will send a pull request to update the lock file to use that new version. This has proven to be quite valuable (to ensure that packages are tested against the most recent versions and whatnot).

Unfortunately, the current behavior (updating only the lock file) causes some problems when the package is released since consumers will not always be grabbing the same version of the dependency as is used by the package's own CI.

Suggested remediation: for dependencies (less of an issue for development dependencies, but still fine if its easier to match behaviors) always update the version range in package.json when updating the lock file...


Take for example, this series of events:

  1. An in-range update of a package's dependency (awesome-thing-a) is released
  2. Dependabot detects the update and sends in a PR updating the package's lock file to point at the new version
  3. CI passes for the PR from dependabot, and is merged (either auto or by the maintainer)
  4. Some time passes
  5. The package authors notice that there is a new awesome feature added by awesome-thing-a that they can take advantage of, and update the package to use the feature (CI passing because the lock files already have the newest version)
  6. The package releases a new version
  7. Users of the package that also use a lock file update to the newly released version
  8. 💥 - since the package didn't update the minimum version in package.json but is now implicitly relying on the new features added in awesome-thing-a, consumers of the package that themselves use lock files will not get the updated version of awesome-thing-a and therefore will break when running the package code that relies on the new feature

Concrete example:

https://github.com/rwjblue/ember-qunit-codemod/pull/123 fixes a real issue for ember-qunit-codemod users (fixes a bug when parsing with recast. If ember-qunit-codemod merges this PR and releases a version, end users are only guaranteed to get a minimum of 0.2.1 (see package.json entry here).

greysteil commented 6 years ago

Thanks for this writeup @rwjblue. It makes perfect sense.

Historically, Dependabot has favoured lockfile-only updates for JS libraries, whist performing lockfile and manifest updates for JS applications. The thinking was that libraries would prefer to keep their dependency requirements wide, even for a language that mainly uses nested dependency resolution, because some users might be using Yarn's --flat. Similarly, although less critically, it also allows npm / Yarn to optimise the size of the node_modules pulled down more effectively.

The downside of the above approach is exactly as you mention - the package doesn't control exactly which dependencies are installed when it specifies a range, and it's hard for Dependabot to know exactly when it should be changing the range. In the PR that you link to it's pretty clear the version requirement should be changed, but hard for Dependabot to distinguish between that and a new feature in a sub-dependency that isn't (yet) relied upon, and shouldn't stop resolution.

My hunch is that Dependabot should switch its behaviour here. I don't think anyone's using --flat, and as long as packages specify their requirements with a SemVer specifier (^) Yarn and npm will still be able to make their optimisations for minor and patch updates. The only question I have is whether this needs to be configurable. What do you reckon? Are you aware of JS projects that try to keep wide ranges? Perhaps to support multiple node versions?

rwjblue commented 6 years ago

I don't think anyone's using --flat, and as long as packages specify their requirements with a SemVer specifier (^) Yarn and npm will still be able to make their optimisations for minor and patch updates.

Yes, exactly my thoughts as well.

The only question I have is whether this needs to be configurable.

This is a hard one to answer, my gut feeling is that it probably doesn't need to be configurable. The footguns that I laid out in the main issue description are just too hard to avoid with the current system.

Are you aware of JS projects that try to keep wide ranges? Perhaps to support multiple node versions?

No, I'm not aware of packages doing this (keeping a very lenient dependency range to allow easier/better deduplication by the package manager) for dependencies (though it is a fairly common technique for peerDependencies which are not really a factor for Dependabot just yet AFAICT).

Perhaps to support multiple node versions?

I don't even think this scenario would work, because yarn and npm do not seem to be smart enough to pick the highest number in an allowed range that works for the current engine version. They both would just grab the highest allowed in the range (and possibly error when that didn't allow the engine in use).

Hypnosphi commented 6 years ago

Looks like choosing "Increase version (always)" in "Update strategy for package.json" should help

greysteil commented 6 years ago

It will, yes. I've kept this open because I still intend to switch it to the default (but need to manage the process to not affect existing users.

Nargonath commented 5 years ago

@Hypnosphi which settings are you referring to? I can't find it.

greysteil commented 5 years ago

It's in the dropdown under "Update strategy for package.json":

image

Nargonath commented 5 years ago

Hum I don't have that interface you are showing (or at least I can't find it) as I used .dependabot/config.yml to configure my project. In case you're wondering here is the link to the aforementioned project: https://github.com/Nargonath/ngt-scripts.

greysteil commented 5 years ago

Ah, for config files you can configure the same using then version_requirement_updates attribute.

Nargonath commented 5 years ago

Oh cool thanks @greysteil. Didn't read the documentation as thorough as I thought.