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

fix(vitest): Fix cacheDir not being honored if deps.optimizer.{mode}.enabled is true #6902

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jacoberdman2147
Copy link
Contributor

@jacoberdman2147 jacoberdman2147 commented Nov 13, 2024

Description

Fixes #6733

So initially it just seemed like the viteConfig.cacheDir set within optimizer should have been prioritizing the one defined in the config but there were other issues with the way cacheDir was being handled within the resolveOptimizerConfig function so I decided to remove the cacheDir logic entirely since it seemed to be unnecessary.

With this, I also fixed an issue where the runVitest function used within some of the tests wasn't properly passing through the options that were given to it. This uncovered the failures within cache.test.ts that should have been happening all along.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Nov 13, 2024

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9f7c89d
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/67353b5cf6dadc0008d19280
😎 Deploy Preview https://deploy-preview-6902--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jacoberdman2147
Copy link
Contributor Author

So I spent a while looking into the test failures and I'm not really sure what to do. The specific web.test.ts and ssr.test.ts work fine before my last commit to remove the resolver plugin's handling of cacheDir, but without that commit we run into issues with cache.test.ts in the default case with optimizer enabled. The issue (with all commits) seems to be that the config for the ViteNodeServer isn't using the resolved cacheDir that we get from the resolveConfig method in the Vitest class, but I'm not sure if it should be or not. I don't completely understand how it all works but it seems like the cache for resolved modules and stuff being based around .vite by default could maybe make sense, but it may also make sense to pass down our resolved cacheDir, I'm not sure.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Nov 15, 2024

Thanks for investigating this. I took a look and indeed the current state is quite complicated. But my intuition is that we shouldn't need to change createVitest. We need to figure out how to deal with cacheDir overwritten here for optimize deps:

viteConfig.cacheDir
= webOptimizer.cacheDir || ssrOptimizer.cacheDir || viteConfig.cacheDir

and we again deriving cache.dir from it

let cacheDir = VitestCache.resolveCacheDir(
'',
resolve(viteConfig.cacheDir, 'vitest'),
resolved.name,
)

Note that Vite's cacheDir for optimize deps and Vitest's cache.dir for ResultCache are two completely different concepts. Probably there's some limits what we can do, but let me try something in #6910.

If we have to choose one, probably test/optimize-deps should pass and cache.test.ts is the one broken.

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.

cacheDir not honored if deps.optimizer.{mode}.enabled is true
2 participants