-
Notifications
You must be signed in to change notification settings - Fork 161
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
feat: add cache pruning #386
Conversation
f946d68
to
b3eb8e9
Compare
README.md
Outdated
// Caches created or modified before `deleteAfter` are not considered for | ||
// deletion. They must be at least this (default: 2 days) old in | ||
// milliseconds. | ||
deleteAfter: 2 * 24 * 60 * 60 * 1000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe options:
cachePrune.maxAge
cachePrune.sizeThreshold
cachePrune
vs prune.
is like "whatever" this is basically just a cache anyway...
92f5a39
to
b2ac142
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty exciting to see this feature get added! Nice one
README.md
Outdated
maxAge: 2 * 24 * 60 * 60 * 1000, | ||
// All caches together must be larger than `sizeThreshold` before any | ||
// caches will be deleted. Together they must be at least this | ||
// (default: 50 MB) in big in bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra "in"
lib/SystemPruneCaches.js
Outdated
|
||
static async fromDirectory(dir) { | ||
const info = new CacheInfo(basename(dir)); | ||
info.lastModified = +(await stat(join(dir, 'stamp'))).mtime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need the +
here? Isn't this returning a date string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a Date object. And the +
is implicitly casting it into a Number. I should do better and use Number(...)
or .getTime()
); | ||
|
||
for (const info of oldInfos) { | ||
rimraf(join(this.cacheRoot, info.id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure the state on rimraf. The build is failing and there's an ignored issue from a month ago with folks wondering what's up with the state of the project. Since you're not really using any of the more "advanced" features of rimraf maybe it's not a dep you need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Looks like the build fail is use of const and rimraf 9 months ago was still testing for node 0.10
and 0.12
support which don't support const
. hard-source
is supporting back to node 8
so I'm not worried about old node incompatibility.
I'm not sure if I want to replace rimraf. hard-source
has been using the current version since December (2017). https://www.npmjs.com/search?q=keywords%3Arm is a fun search to look for alternatives though.
- Add `prune` configuration information to README Prune, by default, caches that are older than 2 days after accumulating 50 megabytes of cache.
Fix #84
Prune, by default, caches that are older than 2 days after accumulating 50 megabytes of cache.