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

Watch dynamically imported files not detected automatically and cached #5429

Closed
6 tasks done
timotheeguerin opened this issue Mar 25, 2024 · 6 comments · Fixed by #5584
Closed
6 tasks done

Watch dynamically imported files not detected automatically and cached #5429

timotheeguerin opened this issue Mar 25, 2024 · 6 comments · Fixed by #5584
Labels
p2-to-be-discussed Enhancement under consideration (priority)

Comments

@timotheeguerin
Copy link

Describe the bug

When dynamically importing JS or TS files (await import with a resolved value) vitest watch doesn't pickup on change automatically and in the case of JS files that are in the dist directory it won't reload at all(Keeps the original code).
The same also happen when working in a vitest workspace and trying to dynamically import JS files from another dependency dist folder.

Reproduction

Stackbliz url

  1. Start a separate terminal and run npm run watch to also build the TS -> JS in dist dir.
  2. All the test should pass
  3. Update foo.ts so the test doesn't pass e.g. export const test = (n: number) => n * 4;
  4. Watch didn't run automatically (That is the first problem)
  5. Press enter in the watch window
  6. Test are now rerunning but only the TS import ones seems to be reloaded. JS files are not even though I reconfigured the exclude to not exclude dist folder. If you change so that TS output dir is customDist instead of dist then the JS files do get reloaded correctly.

System Info

System:
    OS: macOS 14.4
    CPU: (10) arm64 Apple M1 Pro
    Memory: 47.03 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.1 - ~/.nvm/versions/node/v20.11.1/bin/node
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.1/bin/npm
    pnpm: 8.15.4 - ~/.nvm/versions/node/v20.11.1/bin/pnpm
    bun: 1.0.11 - ~/.bun/bin/bun
  Browsers:
    Edge: 123.0.2420.53
    Safari: 17.4
  npmPackages:
    @vitest/coverage-v8: ^1.4.0 => 1.4.0 
    @vitest/ui: ^1.4.0 => 1.4.0 
    vitest: ^1.4.0 => 1.4.0

Used Package Manager

npm

Validations

@hi-ogawa
Copy link
Contributor

Thanks for the concise reproduction. It might look surprising, but I think it's mostly working as intended. Let me check one by one.

  1. Update foo.ts ...
  2. Watch didn't run automatically (That is the first problem)

This looks like because of "too dynamic" dynamic import, which is not statically analyzed by Vite/Vitest, so module graph cannot track "test file -> foo.ts" dependency to trigger auto re-run.

I made a quick example to show the difference here.
https://stackblitz.com/edit/vitest-dev-vitest-giqgmf?file=test%2Fdynamic-ok.test.ts

Given these two tests, bar.ts only triggers re-running dynamic-ok.test.ts:

// dynamic-ok.test.ts
await import('../src/bar');

// dynamic-not-ok.test.ts
await import(String('../src/bar'));  
  1. ... JS files are not even though I reconfigured the exclude to not exclude dist folder. If you change so that TS output dir is customDist instead of dist then the JS files do get reloaded correctly.

This one looks weird, but maybe a different issue, something related to deprecated test.watchExclude option and Vite side server.watch.ignored #5171, which picks up build.outDir: "dist" automatically as ignored.

Actually this might be a bug, so this needs to be investigated further.


Regarding point 4, do you actually use this "too dynamic" form of dynamic import? I think supporting this is out-of-scope for now. But depending on the use case, there might be a workaround, for example, using Vite's import glob import.meta.glob("/dist/*.js"), which can probably keep track module graph correctly.

@timotheeguerin
Copy link
Author

timotheeguerin commented Mar 26, 2024

Thanks for the quick reply, we do actually use a lot of those dynamic loading but only directly to JS for now(I could get into the details but not sure this is really relevant, let me know if you want to), not having the auto rerun of vitest watch is unfortunate but not the end of the world. The main issue I have is it looks like the new vitest extension preview depends on watch and rerunning the test doesn't reload the the dynamically loaded JS files. However that might just be the 2nd issue that you said sounds like a bug. I played also with the server.watch.ignore and couldn't get it to work either.

I'll look into import.meta.glob("/dist/*.js") I didn't know this was a thing thanks!

@AriPerkkio
Copy link
Member

Does forceRerunTriggers work here? https://vitest.dev/config/#forcereruntriggers

@timotheeguerin
Copy link
Author

timotheeguerin commented Mar 26, 2024

It doesn't surprisingly not even in the TS case, I updated the repro abvoe with

    forceRerunTriggers: ['src/**'],

or with

    forceRerunTriggers: ['src/**', 'dist/**'],

neither seems to retrigger the auto run

@AriPerkkio
Copy link
Member

AriPerkkio commented Mar 26, 2024

I thought that would work... Weirdly it looks like it's not possible to make Vite's watcher to look for changes in **/dist/**.

Even something like this doesn't work:

export default defineConfig({
  server: {
    watch: {
      ignored: ["**/node_modules/**"],
    },
  },
  test: {
    watchExclude: ["**/node_modules/**"],
    forceRerunTriggers: ["**/dist/**"],
  },
});

The watcher here still has some hardcoded values in ignored:

const watcher = this.server.watcher

[
  '**/.git/**',
  '**/node_modules/**',
  '**/test-results/**',
  '/x/y/vitest-dist-rerun/node_modules/.vite/**',
  '**/node_modules/**',
  '**/node_modules/**',
  '/x/y/vitest-dist-rerun/dist/**'
]

The build.outDir is always excluded from watcher:

https://github.com/vitejs/vite/blob/c2d0b881d8dcca93e16aa74792e01ecbf3e2bf29/packages/vite/src/node/watch.ts#L13-L24

@timotheeguerin
Copy link
Author

timotheeguerin commented Mar 28, 2024

I see so if I add this to the config then the js reloads fine(though unsure if this is a viable workaround, feel like it should as vitest doesn't actually write to the outDir)

  build: {
    outDir: 'dummy',
  },

also noticed that forceRerunTriggers: ['src/**'], actually makes things worse as now the TS doesn't get reloaded when you press ENTER.

timotheeguerin added a commit to microsoft/typespec that referenced this issue Apr 1, 2024
Setup a workaround for the fact that vitest(vite underneath) doesn't
ever want to reload files in the outDir
vitest-dev/vitest#5429

as the outDir isn't actually used in vitest(as far as I understand) this
is just an ok workaround.
@sheremet-va sheremet-va added p2-to-be-discussed Enhancement under consideration (priority) and removed pending triage labels Apr 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants