Skip to content
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

Deduplicate Yarn Dependencies #5830

Open
1 task done
Kurt-von-Laven opened this issue Oct 3, 2022 · 24 comments · May be fixed by #9388
Open
1 task done

Deduplicate Yarn Dependencies #5830

Kurt-von-Laven opened this issue Oct 3, 2022 · 24 comments · May be fixed by #9388
Labels
L: javascript:yarn npm packages via yarn T: feature-request Requests for new features

Comments

@Kurt-von-Laven
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Feature description

See #1297 (comment) and below for the original discussion of this topic. As proposed in #1297 (comment), one possible compromise would be to dedupe if and only if there are no preexisting duplicates (meaning yarn dedupe runs clean prior to bumping).

@Kurt-von-Laven Kurt-von-Laven added the T: feature-request Requests for new features label Oct 3, 2022
@jeffwidman jeffwidman added the L: javascript:yarn npm packages via yarn label Oct 6, 2022
@jurre
Copy link
Member

jurre commented Oct 6, 2022

What do you think about deduping only for the dependencies that are updated in that PR? Essentially, over time you would end up in a dedupe'd state without a giant diff of unrelated changes on a single (first) PR, and no new duplicates would be introduced, so if you're already in a deduped state, Dependabot doesn't mess that up.

^ would be very simple to implement and I think it could be a good compromise?

@Kurt-von-Laven
Copy link
Contributor Author

That seems reasonable assuming the deduping applies transitively since most duplicates aren't direct dependencies.

@jurre
Copy link
Member

jurre commented Oct 11, 2022

That seems reasonable assuming the deduping applies transitively since most duplicates aren't direct dependencies.

Yeah that's a good question, I'll dig into that

Doesn't seem like that's the case :(

Can you walk me through why Dependabot should do this dedupe step in your opinion? The Yarn docs have a disclaimer on the command, which makes me think it might not be a great default?

Note: Even though it never produces a wrong dependency tree, this command should be used with caution, as it modifies the dependency tree, which can sometimes cause problems when packages don't strictly follow semver recommendations. Because of this, it is recommended to also review the changes manually.

Would it maybe make sense for this to be an Action that folks can set up to run whenever the yarn.lock has changed when they feel it's appropriate?

@kachkaev
Copy link

I wonder if this can be implemented upstream, i.e. in Yarn itself: yarnpkg/berry#4976

@Kurt-von-Laven
Copy link
Contributor Author

Yarn has already declined this feature request. They are open to a plugin that does what you propose, but that wouldn't help Dependabot.

@kachkaev
Copy link

kachkaev commented Oct 20, 2022

They haven't closed yarnpkg/berry#4976 yet depsite pointing to an older discussion (yarnpkg/berry#2770). IMHO Dependabot is a pretty convincing case to tip the scales, so it’d be great if someone from the team could contribute to the discussion.

@jtbandes
Copy link
Contributor

jtbandes commented Nov 1, 2022

We'd like this too for foxglove/studio. The LFS change has gotten us almost there but we still need a manual step to run yarn dedupe: #5830

jtbandes added a commit to foxglove/studio that referenced this issue Nov 1, 2022
**User-Facing Changes**
None

**Description**
Runs `yarn dedupe` automatically on dependabot PRs, a workaround for
dependabot/dependabot-core#5830

Partially reverts #1407
@wojtekmaj
Copy link

wojtekmaj commented Nov 8, 2022

Here's what I came up with.

I run Github Actions workflow on dependabot/** branches, detect working directory, run yarn dedupe on it, commit the changes as GitHub Actions bot.

https://github.com/wojtekmaj/react-date-picker/blob/v10.0.3/.github/workflows/dependabot-dedupe.yml

@steverep
Copy link

steverep commented Dec 7, 2022

Does dependabot run configured precommit hooks for a repository? If so, you could just run yarn dedupe whenever yarn.lock is staged.

@Kurt-von-Laven
Copy link
Contributor Author

I can't imagine that Dependabot deliberately executes arbitrary user-provided code.

@steverep
Copy link

I can't imagine that Dependabot deliberately executes arbitrary user-provided code.

Why not? GitHub does it all the time inside the safety of action runners.

@defunctzombie
Copy link
Contributor

For our CI job we run yarn install --immutable which fails on dependabot PRs that have not been properly de-duped.

As a result, we setup a similar workflow to what @wojtekmaj described: https://github.com/foxglove/studio/blob/main/.github/workflows/dependabot-fix.yml

We also grab the dependabot PR, run dedupe and commit those changes to the dependabot branch. We'd find the dependabot UX improved if we could avoid having to do this manual fixup action and instead could configure dependabot to run the dedupe command since it has already run the yarn commands to do the upgrades.

@Kurt-von-Laven
Copy link
Contributor Author

Why not? GitHub does it all the time inside the safety of action runners.

I suspect that Dependabot has more permissions than a typical GitHub Action.

@oliversalzburg
Copy link

@wojtekmaj @defunctzombie How do you handle the problem of Dependabot refusing to update a PR that has been deduped like that? I now have dozens of open Dependabot PRs that are in conflict and abandoned.

@defunctzombie
Copy link
Contributor

@oliversalzburg I linked the workflow we use: https://github.com/foxglove/studio/blob/main/.github/workflows/dependabot-fix.yml This runs on all PRs made by dependabot to dedupe.

@oliversalzburg
Copy link

oliversalzburg commented Feb 8, 2023

Yeah, that's what I copied, but I adjusted the Name/Email of the bot. Maybe that's the problem. I didn't assume you could just imitate Dependabot that easily.

I changed the name and email to the information I got from previous Dependabot commits in our git history now. Hopefully, that resolves the conflicts I was seeing. Thanks :)

@jtbandes
Copy link
Contributor

jtbandes commented Feb 8, 2023

@oliversalzburg see https://docs.github.com/en/code-security/dependabot/working-with-dependabot/managing-pull-requests-for-dependency-updates#allowing-dependabot-to-rebase-and-force-push-over-extra-commits

@Kurt-von-Laven
Copy link
Contributor Author

It seems like this issue has been addressed. Will close it pending confirmation.

@defunctzombie
Copy link
Contributor

@Kurt-von-Laven Do you have a link to any PR or docs that indicate this is built-in now?

@Kurt-von-Laven
Copy link
Contributor Author

No, sorry. Just an empirical observation based on a private repository since we use Forking Renovate for our open-source projects.

@Kurt-von-Laven
Copy link
Contributor Author

Apologies; it appears I was wrong. We recently encountered a counterexample where the Yarn dependencies were not deduplicated.

@simPod
Copy link

simPod commented Oct 18, 2023

I can confirm it does not dedupe.

@Kurt-von-Laven
Copy link
Contributor Author

It does sometimes but not always, which is what threw me off.

unicornware added a commit to flex-development/grease that referenced this issue Oct 22, 2023
- dependabot/dependabot-core#5830
- partially replaces `dependabot-auto`

Signed-off-by: Lexus Drumgold <unicornware@flexdevelopment.llc>
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
mingjunlu added a commit to mingjunlu/mingjun.lu that referenced this issue Feb 6, 2024
@smorimoto
Copy link

This issue should be fixed in this PR: #9388

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: javascript:yarn npm packages via yarn T: feature-request Requests for new features
Projects
None yet
Development

Successfully merging a pull request may close this issue.