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

Support .ava cacheDir for better compatibility with Yarn PnP #3226

Open
andrewzey opened this issue Jul 28, 2023 · 3 comments
Open

Support .ava cacheDir for better compatibility with Yarn PnP #3226

andrewzey opened this issue Jul 28, 2023 · 3 comments

Comments

@andrewzey
Copy link

Hello,

We're using Yarn 3 with PnP in our mono-repo, and find that ava is creating unwanted node_modules directories in our workspace (with node_modules/.cache/ava/failing-tests.json) This is a result of

ava/lib/cli.js

Line 256 in f047694

const cacheDir = path.join(projectDir, 'node_modules', '.cache', 'ava');

Sometimes the existence of node_modules confuses the pnp patched executables (eg. eslint), so the mere existence of node_modules can sometimes cause issues with things working reliable.

It would be great if you supported some manner of specifying the cache directory or simply defaulted to projectDir/.ava and recommended adding .ava to the .gitignore.

Thanks!

@andrewzey andrewzey changed the title Consider allowing specifying cacheDir for better compatibility with Yarn PnP Consider allowing specifying cacheDir or at least no node_modules for better compatibility with Yarn PnP Jul 28, 2023
@novemberborn
Copy link
Member

I was thinking about this the other day actually, and yes I agree that should be an option. Placing it under node_modules by default does provide the best out-of-the box experience though.

cache is currently configurable as a boolean, I propose we add '.ava' as a valid value which would then use path.join(projectDir, '.ava') as the cache dir. All other values should result in a configuration error, I don't think we should make this super configurable just yet.


However this does leave us with the following code:

ava/lib/worker/base.js

Lines 210 to 217 in 4dc385b

let importFromProject = async ref => {
// Do not use the cacheDir since it's not guaranteed to be inside node_modules.
const avaCacheDir = joinPath(projectDir, 'node_modules', '.cache', 'ava');
await mkdir(avaCacheDir, {recursive: true});
const stubPath = joinPath(avaCacheDir, 'import-from-project.mjs');
await writeFileAtomic(stubPath, 'export const importFromProject = ref => import(ref);\n');
({importFromProject} = await import(pathToFileURL(stubPath)));
return importFromProject(ref);

The way this could work is:

  • If cache is not false, use the computed cacheDir
  • Otherwise, insist on using this node_modules/.cache/ava directory

And then we'd have to document that as a caveat.

What do you think?

@novemberborn novemberborn changed the title Consider allowing specifying cacheDir or at least no node_modules for better compatibility with Yarn PnP Support .ava cacheDir for better compatibility with Yarn PnP Jul 30, 2023
@andrewzey
Copy link
Author

Wow, sorry about the incredibly delayed response!

I'm not a huge fan of polymorphic variables personally, so it seems cleaner to leave cache as a boolean and have an additional cacheDir configuration option that has an allowlist of values, starting with just ['.ava'] as you proposed and defaulting to node_modules/.cache/ava if the option isn't specified.

Would you be ok with that?

@novemberborn
Copy link
Member

@andrewzey that's fair. Maybe tempDir would be a better name? Or overrideTempDir?

Thinking about the importFromProject function I mentioned last year, I think that should still be fine as long as this directory is within the project, and that can be validated. Though that wouldn't be necessary if it only accepts known values.

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

No branches or pull requests

2 participants