Run yarn dedupe entirely instead of specific package(s)

[smorimoto]

Closes #5830

[jurre]

We chose specifically to only dedupe the package being updated to keep the diff Dependabot generates as small as possible.

This would result in Dependabot making changes unrelated to the dependency being updated and I am not sure that it's the right thing to do.

There should be previous discussions about this in the issue tracker as well. See #5830

I'm curious to hear some more reasons about why you think this would be preferable, and why it's Dependabots responsibility to do this

[tierra]

The existing dedupe definitely isn't covering all situations that result in duplicate dependencies being introduced specifically by the same Dependabot bumps. I still quite regularly have to manually run a full dedupe on Dependabot PRs to pass dedupe checks.

Does the existing dedupe include all downstream dependencies of the package(s) being bumped? It doesn't seem like it does.

I subscribed to this issue (and PR) initially without even realizing that it does apparently run that (restricted) dedupe because I thought it just didn't run dedupe at all, given how often I have to manually address these.

An alternative approach that would be helpful is exposing a new optional setting to perform a full dedupe instead, thus still offering the current approach by default (assuming this is actually working as desired for most projects).

[deivid-rodriguez]

Maybe Dependabot could try a dedupe before the update, and then do the following:

• If that dedupe does not cause any lockfile changes, then run a full dedupe after the update since any duplications will have been caused by Dependabot for sure.
• If that dedupe already causes lockfile changes, then don't run dedupe at all since the original lockfile was already not deduped.

In any case, it'd be good to see a realworld example of Dependabot creating duplications, also to verify that the proposed approach fixes it.

[benjie]

In any case, it'd be good to see a realworld example of Dependabot creating duplications, also to verify that the proposed approach fixes it.

Here's an example:

https://github.com/graphile/gatsby-source-pg-example/pull/34/files

It upgraded express, which resulted in an upgrade of body-parser from 1.20.1 to 1.20.2, but it left the previous version of body-parser around causing a duplicate that wasn't previously present.

[smorimoto]

@jurre Rather than keeping the diff small, it is important for this kind of tool to minimise duplication of dependencies and keep the number of dependencies and their variants as minimal as possible. This often reduces bundle size and dependency installation time, which is probably the behaviour developers want in many cases.

@tierra I'm not really in favor of adding new options. Because there's no reasonable reason to allow duplicate dependencies, it doesn't feel like a good idea to make the implementation of Dependabot more complicated than it needs to be.

[deivid-rodriguez]

@jurre Rather than keeping the diff small, it is important for this kind of tool to minimise duplication of dependencies and keep the number of dependencies and their variants as minimal as possible. This often reduces bundle size and dependency installation time, which is probably the behaviour developers want in many cases.

If you think this is something everybody benefits from, perhaps it'd be a good idea to propose it upstream. Then Dependabot would just work and no dedupe would be necessary.

@deivid-rodriguez I'm not really in favor of adding new options. Because there's no reasonable reason to allow duplicate dependencies, it doesn't feel like a good idea to make the implementation of Dependabot more complicated than it needs to be.

I probably failed to explain my idea, since I didn't propose to add any new options 🤔. I proposed to make things just work for everyone. Also if I recall correctly, Dependabot already checks for resolvability before the update, and uses similar criteria to try keep initial lockfile shape as much as possible. So I don't think my idea particularly increases current complexity.

[smorimoto]

@deivid-rodriguez Oops! I left the wrong mention! Do you mean the Yarn project by "upstream"?

[deivid-rodriguez]

Oh, I see, now everything makes more sense :)

Yes, my point is that maybe the Yarn project is interested in deduplicating by default. Not sure, but maybe worth asking around!

[graup]

I think @deivid-rodriguez's proposal sounds very reasonable. Dependabot could maybe even output a warning in case the original lockfile was not deduped, as a nudge. IME you almost never want duplicates and almost always want dedupe.

[deivid-rodriguez]

I'd be hesitant to show any warning though, because it feels it's not Dependabot's job to enforce this kind of thing. If according to Yarn it's a bad practice to allow lockfile duplicates, then it sounds like it should be Yarn itself that deduplicates by default, and warns when lockfile has duplicates. It seems everybody here agree that duplicates are bad, so I'd take this to upstream Yarn.

[eelco]

I can already predict that this will not soon be fixed upstream, because the docs at https://yarnpkg.com/cli/dedupe clearly explain the reasoning here and there is no/low appetite for changing that: yarnpkg/berry#4976

I personally would appreciate if Dependabot took a more practical approach to these types of issues, like Renovate does: https://docs.renovatebot.com/configuration-options/#postupdateoptions

[flying-sheep]

Another real-world example: flying-sheep/phil.red@714107d (#567)

before, it’s

"postcss@npm:^8.4.31, postcss@npm:^8.4.41":
  version: 8.4.41
  ...

after, it’s

"postcss@npm:^8.4.31":
  version: 8.4.41
  ...

"postcss@npm:^8.4.43":
  version: 8.4.47
  ...

but I want to see

"postcss@npm:^8.4.31, postcss@npm:^8.4.43":
  version: 8.4.47
  ...

my workaround is https://github.com/ambar/yarn-plugin-dedupe-on-install