Autoclean local cache folder #372
Conversation
| ➤ YN0000: ┌ Link step | ||
| ➤ YN0000: └ Completed | ||
| ➤ YN0000: ┌ Cache cleanup step | ||
| ➤ YN0054: │ skipping cleanup since cache folder is not inside project folder |
There was a problem hiding this comment.
🥕 incentive to use local cache
There was a problem hiding this comment.
A few suggestions but it's pretty good already 👍
A followup might be to update yarn cache clean to follow a similar architecture (although it'll require to add an option to the cache to prevent it from fetching the files if they don't already exist, so it should be done in a different PR).
| @@ -1142,6 +1143,10 @@ export class Project { | |||
| await this.linkEverything(opts); | |||
| }); | |||
|
|
|||
| await opts.report.startTimerPromise(`Cache cleanup step`, async() => { | |||
| await this.cacheCleanup(opts); | |||
There was a problem hiding this comment.
I think this should be done in the fetch step, right after fetchEverything. I'd like to keep the number of steps low because it's easier to teach, and less verbose in terms of output 🤔
| return; | ||
|
|
||
| if (!isFolderInside(cachePath, this.cwd)) | ||
| return report.reportInfo(MessageName.CACHE_OUTSIDE_PROJECT, `skipping cleanup since cache folder is not inside project folder`); |
There was a problem hiding this comment.
Ahah yeah incentive, but at the same time if they've explicitly configured the cache to be stored elsewhere then I think we should trust that they know what they're doing and keep our "verbosity budget" low 😄
| @@ -1252,6 +1257,35 @@ export class Project { | |||
| await workspace.persistManifest(); | |||
| } | |||
| } | |||
|
|
|||
| async cacheCleanup({cache, report}: InstallOptions) { | |||
| const cachePath = cache.cwd; | |||
There was a problem hiding this comment.
Can we rather use cache.cwd when we need it? It's what we're doing in the rest of the code with things like workspace.cwd etc.
|
|
||
| await run(`install`, {env}); | ||
|
|
||
| expect(await readdir(sharedCachePath)).toMatchSnapshot(); |
There was a problem hiding this comment.
I'd prefer to avoid using snapshots if possible - can you replace the readdir snapshots by a number of files check?
| @@ -18,3 +18,9 @@ export function getDefaultGlobalFolder() { | |||
| export function getHomeFolder() { | |||
| return NodeFS.toPortablePath(homedir() || '/usr/local/share'); | |||
| } | |||
|
|
|||
| export function isFolderInside(target: PortablePath, parent: PortablePath) { | |||
There was a problem hiding this comment.
I wonder if Node would accept a PR to add a similar function to path ...
Co-Authored-By: Maël Nison <nison.mael@gmail.com>
| }, async ({path, run, source}) => { | ||
| const sharedCachePath = await createTemporaryFolder(); | ||
| const env = { | ||
| YARN_CACHE_FOLDER: sharedCachePath |
There was a problem hiding this comment.
Fyi (not a blocker), it also can be done by writing a .yarnrc.yml file into path. We do this in a few other places (mainly for the plugins).
What's the problem this PR addresses?
Autoclean local cache
How did you fix it?
Added a Cache cleanup step to install, which should work on
install,add,up,remove.Which packages would need a new release (if any)?
Have you run
yarn version [major|minor|patch] --deferredin those packages?Closes #247, #228