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

Bizarre tsconfig hijacking #359

Closed
4 tasks done
echocrow opened this issue Apr 18, 2024 · 5 comments · Fixed by vitest-dev/vitest#5582
Closed
4 tasks done

Bizarre tsconfig hijacking #359

echocrow opened this issue Apr 18, 2024 · 5 comments · Fixed by vitest-dev/vitest#5582

Comments

@echocrow
Copy link

echocrow commented Apr 18, 2024

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:

  • disable this VSCode extension, or
  • remove the root vitest.workspace.js file, or
  • remove the vite.config.ts file of sibling packages, or
  • uninstall the vitest package at the root package

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
Copy link
Contributor

hi-ogawa commented Apr 19, 2024

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 vitest-dev/vitest#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
Copy link
Author

echocrow commented Apr 19, 2024

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:

  • The extension actively reads and executes vite.config.ts files pointed to in vitest.workspace.js, e.g. when package.json is modified. The configs are invoked in such a way that trigger the configureServer API hook of Vite plugins listed in the config. (And as you stated, cwd at this point could be the root directory.)

SvelteKit

  • SvelteKit's registration of its Vite plugin is not free of side effects. When the configureServer Vite plugin API hook is invoked, this plugin actively scans the SvelteKit setup, compiles its own tsconfig, and writes files to disk. I would have expected this to only happen during dev or build. Maybe the assumption was/is that configureServer would only be called during dev/build. ATM I don't know enough to say whether causing side effects during configureServer is best practice or bad practice. (Again, as you stated, process.cwd may be used during that call, which is expected to be the SvelteKit package dir, but that's not guaranteed.)

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
Copy link
Member

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
Copy link
Contributor

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 vitest-dev/vitest#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
Copy link
Author

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 vitest-dev/vitest#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. 🫠

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

Successfully merging a pull request may close this issue.

3 participants