-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Clarify caching #2794
Clarify caching #2794
Conversation
Hi @mcecode, thanks for the PR! Unfortunately I haven't had a chance to look at it yet. |
It's okay, I know you guys must be busy with the AVA 4 release and other things. So just tell me if I'm missing something or if need to change anything once you get around to review it. Thanks a lot! |
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.
Nice, thanks for this @mcecode! I've left some inline comments. I'll also approve CI now so let's see if that flags anything.
lib/cli.js
Outdated
} | ||
|
||
try { | ||
await del('*', {cwd: cacheDir}); |
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.
Looking at the docs for del
, it returns the paths of the removed files. So we can check if that's empty and update the messaging based on that. I think that's simpler than having a separate check for the directory.
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.
Thanks for pointing that out. I can't believe I missed that 😅, I just defaulted to using fs
. I've made a commit fixing this.
Co-authored-by: Mark Wubben <mark@novemberborn.net>
Co-authored-by: Mark Wubben <mark@novemberborn.net>
Hey @novemberborn, thanks for the feedback! I've added some commits that I think resolve them already. Just looking at the logs, I think the CI failed because the lines I added were not covered by the tests but I'm not so sure. Just tell me if there's anything else I can do. |
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.
Awesome, thanks @mcecode!
Hi! I'm new to AVA and while I was using it I noticed that subsequent runs didn't seem to finish faster even though I wasn't changing some of my test files. This prompted me to try setting up caching, so I set
"cache": true
in mypackage.json
like the docs suggested. However, it didn't seem to make a difference and I couldn't find the specifiednode_modules/.cache/ava
directory where the cache is supposed to be. Turns out, AVA itself doesn't cache files unless used with@ava/babel
. This pull request aims to clarify this, both in the docs and when runningnpx ava reset-cache
.Some notes on the changes I made to the docs:
cache
option is boolean because some (like I did) might think that it can be set to a directory where the cache will then be set.Some notes on the changes I made to the
reset-cache
command:resetCache
because it is redundant as eitherprocess.exit()
orexit()
already terminates the script.console.error()
toconsole.log()
because the message logged isn't an error.nodir: true
option fromdel
as it is not a valid option infast-glob
,fast-glob
instead usesonlyFiles
which is then set totrue
by default.