Skip to content

Add optional offline mirror pruning #2836

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.

Sign up for GitHub

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

Jump to bottom
Merged
bestander merged 1 commit into from
Mar 8, 2017
Merged

Add optional offline mirror pruning #2836

bestander merged 1 commit into from
Mar 8, 2017

Conversation

@dguo
Copy link
Contributor

@dguo dguo commented Mar 5, 2017 •
edited
Loading

Summary

This change is an implementation of yarnpkg/rfcs#49, which stems from #2109.

Test plan

I tested the feature locally with two scenarios where .yarnrc has yarn-offline-mirror-pruning set to true:

  • Upgrading a package:
    • $ yarn add is-positive-integer@1.0.0
    • Check that the offline mirror has the is-positive-integer tarball, along with all its dependencies
    • $ yarn upgrade is-positive-integer@1.1.1
    • Check that the offline mirror only has the new version of is-positive-integer, which doesn't have any dependencies
  • Removing a package:
    • $ yarn add left-pad right-pad
    • Check that the offline mirror has both the left-pad and right-pad tarballs
    • $ yarn remove left-pad
    • Check that the offline mirror only has right-pad

I also added two Jest test cases, one for upgrade and one for remove. They verify that sub-dependencies are also pruned.

For documentation, I'd be happy to submit a pull request for that as well, but I don't see any for the offline mirror feature on the main Yarn website. I've just been referencing this blog post.

@dguo dguo mentioned this pull request Mar 5, 2017
Merged
dguo
dguo commented Mar 5, 2017
View reviewed changes
src/cli/commands/install.js
const requiredTarballs = new Set();
for (const dependency in lockfile) {
const resolved = lockfile[dependency].resolved;
if (resolved) {
Copy link
Contributor Author

@dguo dguo Mar 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it is okay to depend on the format of resolved like this.

Copy link
Member

@bestander bestander Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests would fail if there are breaking changes.
I suppose it should be fine.

@bestander
Copy link
Member

bestander commented Mar 6, 2017

Looks nice, how about a test for add command as well?

@bestander
Copy link
Member

bestander commented Mar 6, 2017

Tests seem to fail for some other reason

@dguo
Copy link
Contributor Author

dguo commented Mar 7, 2017

Yep, I considered adding a test for add as well, but I couldn't come up with a situation where pruning would apply. Maybe there's some uncommon semver combination that I haven't thought of? If not, should I just copy over the upgrade test fixture and use that? So it would simulate:
$ yarn add dep-a@1.0.0
$ yarn add dep-a@1.1.0

The tests fail for me locally as well, but the last I tried, the same tests failed on the master branch. I attributed the failures to flakiness, but would you like me to dig into them?

@bestander
Copy link
Member

bestander commented Mar 7, 2017

I think you are right, add that would cause package remove is only when a package already exists and gets updated.
RE tests, seems like it is flakiness

@bestander
Copy link
Member

bestander commented Mar 7, 2017

@dguo, could you rebase please?
And also could you send a PR to the docs website?
Otherwise no one will ever know about this feature.

@dguo
Copy link
Contributor Author

dguo commented Mar 7, 2017 via email

@bestander
Copy link
Member

bestander commented Mar 7, 2017 via email

@dguo dguo force-pushed the prune-offline-mirror branch from 316b914 to 3779a94 Compare March 8, 2017 03:34
@dguo dguo mentioned this pull request Mar 8, 2017
Merged
@dguo
Copy link
Contributor Author

dguo commented Mar 8, 2017

Ok, it's rebased, the tests passed, and I submitted yarnpkg/website#409 for the docs.

@bestander
Copy link
Member

bestander commented Mar 8, 2017

Great job on pushing through the whole feature from RFC to great implementation with tests and docs, @dguo!

@bestander bestander merged commit 1c36baf into yarnpkg:master Mar 8, 2017
@dguo
Copy link
Contributor Author

dguo commented Mar 8, 2017

Thank you!

@dguo dguo deleted the prune-offline-mirror branch March 8, 2017 14:29
@rmacklin rmacklin mentioned this pull request Mar 9, 2017
Merged
@rmacklin
Copy link
Contributor

rmacklin commented Mar 9, 2017

Wow, awesome! I'm participating in a hackathon today and this was actually the first thing I was gonna work on (I submitted the original issue), but I'm happy to see it's already done. Thanks @dguo!

Also, re: the tests and not having a test for add, I think there's an untested case we may want to cover. Suppose a team has been using yarn with an offline mirror for a while prior to the addition of this feature. Once they enable pruning, unless they had been manually pruning before, there will still be unused tarballs in the offline mirror. At that point, they should run yarn with no arguments (or equivalently yarn install) to prune all those unused tarballs, so that future add/remove/upgrade commands don't get mixed in with unrelated pruning.

So, I think it'd be good to add a test case for this scenario: start with a lockfile and a mirror that has more tarballs than what's in the lockfile, run yarn with no arguments (i.e. yarn install), verify that the unused tarballs were pruned. I've done that here: #2880

We could also add a similar test for running add, remove, and upgrade when the offline mirror already contains unused tarballs unrelated to the package that was added/removed/upgraded (we'd expect them to be pruned), though that may be overkill (if we don't think it's overkill, I can add those tests to the PR).

@rmacklin rmacklin mentioned this pull request Mar 9, 2017
Closed
This was referenced Mar 14, 2017
Closed
Closed
Closed
Closed
Closed
Closed
@rmacklin
Copy link
Contributor

rmacklin commented Mar 31, 2017

Is there any chance this could make it into a (pre-)release soon?

@bestander
Copy link
Member

bestander commented Mar 31, 2017 via email

@rmacklin
Copy link
Contributor

rmacklin commented Mar 31, 2017

Awesome, and thanks for the speedy reply!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bestander bestander bestander left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dguo @bestander @rmacklin