Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.
This repository has been archived by the owner on May 24, 2022. It is now read-only.

Update and minimise transitive JS dependencies #313

Closed
jacobpgn opened this issue Jan 3, 2019 · 10 comments
Closed

Update and minimise transitive JS dependencies #313

jacobpgn opened this issue Jan 3, 2019 · 10 comments

Comments

@jacobpgn
Copy link

jacobpgn commented Jan 3, 2019

👋 💙 🤖

I've found that when you upgrade a package with yarn it won't upgrade transitive dependencies of that package if the existing transitive dep is in range (even if a newer version is available and also in range). This means that you can end up with loads of old versions of a package that could be replaced by fewer new versions.

Reading a few threads on the issue (e.g. yarnpkg/rfcs#54 and yarnpkg/yarn#4986) led me to find https://www.npmjs.com/package/yarn-deduplicate which attempts to replace multiple versions of a dependency with the highest common version.

I think it could be really handy if Dependabot could:

  • attempt to reduce the number of dependency versions (yarn-deduplicate-style)
  • update transitive dependencies in general (not sure how noisy this would be in JS-land, where there are usually a lot)
@feelepxyz
Copy link
Contributor

@jacobpgn 👋

I've been thinking about this too! I've done this in the past by periodically removing my lockfile and re-installing to update transitive dependencies. Definitely something Dependabot should be doing 😃

Dependabot can update transitive dependencies but it's specifically disabled for JS because it would definitely cause lots of noise if we open PRs for each update.

One way I've been thinking of doing this is having a separate cleanup task, possibly triggered from the dashboard to start with, that updates all transitive dependencies in one PR.

Haven't looked into de-duping at all but think we would have to be careful not to accidentally install a vulnerable version of some transitive dependency.

Will let you know when I get some time to properly look at this! ✌️

@feelepxyz
Copy link
Contributor

@jacobpgn actually forgot we already do de-duping for yarn lockfiles and find the maximum satisfied version. Have you found some cases where this didn't happen?

@jacobpgn
Copy link
Author

jacobpgn commented Jan 3, 2019

Sure! Here's an example from a few hours ago (lmk if you can't see that and would need to!), and rimraf has a version which could/should have been dropped.

Before, we required:
rimraf@^2.2.8, rimraf@^2.5.4, rimraf@^2.6.1, rimraf@^2.6.2 -> satisfied by 2.6.2
rimraf@~2.4.0 -> satisfied by 2.4.5

so we have 2 versions installed.

After, we also require 2.6.3 so we got:
[email protected] -> satisfied by 2.6.3
rimraf@^2.2.8, rimraf@^2.5.4, rimraf@^2.6.1, rimraf@^2.6.2 -> satisfied by 2.6.2 ❗️
rimraf@~2.4.0 -> satisfied by 2.4.5

but really 2.6.2 could be dropped! All of those requirements currently satisfied by 2.6.2 would also be satisfied by 2.6.3

@feelepxyz
Copy link
Contributor

@jacobpgn oh yeah that does look suspicious! Looking into it.

@feelepxyz
Copy link
Contributor

@jacobpgn just got a fix in for de-duping transitive deps for yarn - code is here

Going to close this and keep pondering updating transitive deps, still not sure if/how we should do this for JS.

@jacobpgn
Copy link
Author

jacobpgn commented Jan 4, 2019

Cheers Phil, looks good!

For transitive deps, maybe it makes sense to update them when updating a package dep?

That way it wouldn't increase the frequency of bump PRs. You would be less up-to-date than updating transitive dependencies independently and regularly, but more up-to-date than the current approach. The behaviour would be similar to what you get if you yarn remove pkg && yarn add pkg.

@feelepxyz
Copy link
Contributor

@jacobpgn oh yeah that would be nice! Think the only way is doing what you suggested, so yarn remove pkg && yarn add pkg@v - maybe I'm missing something? This will require quite a few changes to the way we update things as it would rewrite the package json which we currently do separately to make sure you can configure the strategy.

How keen are you on this? 🤠

@jacobpgn
Copy link
Author

jacobpgn commented Jan 4, 2019

Yeah from reading around it looks like there's nothing built in to yarn for this (but people want it!)

I'm wondering what happens if you remove in https://github.com/dependabot/dependabot-core/blob/7518ba0e97bc2b5237786e85f48269ea90525ce1/helpers/yarn/lib/updater.js#L181 before the (lightweight) add, but I haven't tried that out yet.

I think it'd be pretty good for keeping the world more up to date, but I don't feel too strongly and haven't seen any other requests for this 👀

@feelepxyz
Copy link
Contributor

Yep feel like this should be built into yarn as the default (also de-duping) when updating individual packages. Going to see if yarn team would be open to this change.

@TildeWill
Copy link

@feelepxyz just felt this pain here @stitchfix. Is there a yarn issue we can upvote?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants