Skip to content
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

Merged
merged 6 commits into from Aug 22, 2021
Merged

Clarify caching #2794

merged 6 commits into from Aug 22, 2021

Conversation

mcecode
Copy link
Contributor

@mcecode mcecode commented Jul 27, 2021

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 my package.json like the docs suggested. However, it didn't seem to make a difference and I couldn't find the specified node_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 running npx ava reset-cache.

Some notes on the changes I made to the docs:

  • I specified that the 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:

  • I removed the return statement at the end of the if check for resetCache because it is redundant as either process.exit() or exit() already terminates the script.
  • I changed console.error() to console.log() because the message logged isn't an error.
  • I removed the nodir: true option from del as it is not a valid option in fast-glob, fast-glob instead uses onlyFiles which is then set to true by default.

@mcecode mcecode marked this pull request as ready for review July 27, 2021 19:57
@novemberborn
Copy link
Member

Hi @mcecode, thanks for the PR! Unfortunately I haven't had a chance to look at it yet.

@mcecode
Copy link
Contributor Author

mcecode commented Aug 5, 2021

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!

Copy link
Member

@novemberborn novemberborn left a 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.

docs/05-command-line.md Outdated Show resolved Hide resolved
docs/05-command-line.md Outdated Show resolved Hide resolved
docs/06-configuration.md Outdated Show resolved Hide resolved
lib/cli.js Outdated
}

try {
await del('*', {cwd: cacheDir});
Copy link
Member

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.

Copy link
Contributor Author

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.

mcecode and others added 4 commits August 9, 2021 01:06
Co-authored-by: Mark Wubben <mark@novemberborn.net>
Co-authored-by: Mark Wubben <mark@novemberborn.net>
@mcecode
Copy link
Contributor Author

mcecode commented Aug 8, 2021

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.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @mcecode!

@novemberborn novemberborn merged commit ddda6ed into avajs:main Aug 22, 2021
@mcecode mcecode deleted the clarify-caching branch August 22, 2021 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants