vitest-dev / vscode

VS Code extension for Vitest
https://vitest.dev/vscode
MIT License
736 stars 83 forks source link

Bizarre tsconfig hijacking #359

Closed echocrow closed 4 months ago

echocrow commented 4 months ago

Describe the bug

We experienced some head-scratching confusion when our SvelteKit app suddenly started complaining about missing route files. After some commit and VSCode extension bisecting, we came across a bizarre behaviour that was causing SvelteKit's internal tsconfig.json to be overridden (seemingly by the Vitest VSCode extension) when a combination of criteria are met.

Specifically, various paths in compilerOptions.paths and compilerOptions.rootDirs get updated, whenever any package.json file is modified in any way, replacing those paths with invalid paths to another package (that happens to also have Vite config) in the same monorepo. This happens seemingly "magically", even when no dev or other scripts are running.

Charlie from It's Always Sunny in Philadelphia going crazy

Reproduction

Repro, along with demo steps, can be found in this repo:

This repro uses SvelteKit for demoing purposes. I believe this issue is not specific to SvelteKit, however.

The issue goes away as soon as any one of the following steps are taken:

Weirder still, most of these steps will also "reset"/restore the hijacked tsconfig.json file to its original, expected state.

Output

[INFO 2:36:55 PM] [API] Vitest process 33901 closed successfully
[INFO 2:36:55 PM] [API] Running Vitest: v1.5.0 (vitest.workspace.js)
[INFO 2:36:55 PM] [API] Starting Vitest process with Node.js: /path/to/library/pnpm/node
[INFO 2:36:55 PM] [API] Vitest process 33977 created

Version

v0.8.4

Validations

hi-ogawa commented 4 months ago

This is probably the same issue as:

Since sveltekit has write_tsconfig(.., process.cwd()), so depending on where Vitest is chdir-ed, they read a different user tsconfig. It looks like they are calling this during configureServer, which is after configResolved where Vitest chdir to the next project https://github.com/vitest-dev/vitest/pull/5476 I think technically Sveltekit can fix this by replacing process.cwd with vite_config.root somehow.

https://github.com/sveltejs/kit/blob/e7489aa558244ff0a356b07c62199f25e7afb025/packages/kit/src/core/sync/write_tsconfig.js#L36-L47

With this assumption, the same thing should reproduce from Vitest CLI, so I may be missing something.

echocrow commented 4 months ago

Since sveltekit has write_tsconfig(.., process.cwd()), so depending on where Vitest is chdir-ed, they read a different user tsconfig.

Sounds reasonable and could be a good way of solving this issue!


The thick plottens.

With the help of your pointer, and some more tracing, I think I'm starting to see what's going on here. What I neither knew nor expected before, but what I now believe is happening:

Vitest VSCode extension:

SvelteKit

TL;DR: Mix those two together, and you seem to get the following trace:

  1. Vitest VSCode extension runs in the background.
  2. A wild package.json file appears and is modified.
  3. Vitest VSCode extension is triggered, which in turn...
    1. reads vitest.workspace.js and parses its file globs,
    2. reads Vite configs contained in the above globs, and
    3. invokes the Vite configs in such a way that triggers async configureServer plugin hooks.
  4. SvelteKit's configureServer is invoked, which generates a nested tsconfig.json file
    1. This code generation currently depends on process.cwd, which is expected to the be SvelteKit's package dir. However, by the time this is read (async code and all), process.cwd could be pointing to another dir or the root dir.

If not already reported, I'll report the likely problematic use of process.cwd to the SvelteKit project.

One lingering question though: Is it expected that this VSCode plugin reads Vite configs and invokes configureServer plugin hooks?

sheremet-va commented 4 months ago

One lingering question though: Is it expected that this VSCode plugin reads Vite configs and invokes configureServer plugin hooks?

Yes, this is completely normal behaviour. The extension restarts the whole process to do it. This is also what Vitest does when root package.json is updated.

I guess what the extension should do is only react to package.json that is related to the process. Currently it just restarts Vitest when any package.json is updated (which is different from Vitest CLI).

The other thing that needs to be fixed is on the Vitest side where we should await the workspace project resolution instead of switching the CWD to the next project.

hi-ogawa commented 4 months ago

TL;DR: Mix those two together, and you seem to get the following trace: ... I'll report the potentially problematic process.cwd in the SvelteKit repo (if not already reported).

Yeah, that's my rough guess, but I said "technically" because I'm not so confident to say this is a sveltekit bug instead of Vitest bug. As seen in the issue I linked https://github.com/vitest-dev/vitest/issues/5541, there might be others plugins affected by the current behavior.

For example, like sheremet-va just mentioned, Vitest could figure out the better timing to switch chdir based on how plugins commonly do weird side-effects.

echocrow commented 4 months ago

cheers for the all the added context!

it's seems clearer now that this bug is not originating from the VSCode extension at all. (Apologies for the miss-attribution.) I can confirm I'm seeing the same issue running vitest directly.

I've updated the repro to demonstrate this, independent of the IDE or IDE extension.

The other thing that needs to be fixed is on the Vitest side where we should await the workspace project resolution instead of switching the CWD to the next project.

Yeah this could also fix this issue, and potential issues in other plugins (that use process.cwd), too. I assume this would marginally slow down the resolution (running workspace server creation in serial rather than parallel). But maybe this tradeoff for the heightened stability is worthwhile?

Yeah, that's my rough guess, but I said "technically" because I'm not so confident to say this is a sveltekit bug instead of Vitest bug. As seen in the issue I linked https://github.com/vitest-dev/vitest/issues/5541, there might be others plugins affected by the current behavior.

Makes sense. I suppose whether this is a bug in SvelteKit/other-vite-plugins bug or in Vitest depends on whether you'd expect process.cwd to be stable (pointing to the package dir) or volatile (instructing to use e.g. config.root instead).

From experience I'd lean towards avoiding process.cwd when possible. In this case, config.root also feels more declarative. But this does put the burden onto more lib maintainers. 🫠