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

Errors in config file may result in a huge amount of temporary files being generated #403

Open
Dschungelabenteuer opened this issue Oct 26, 2024 · 0 comments

Comments

@Dschungelabenteuer
Copy link

Hey 👋

Please forgive me in advance, I'm not familiar at all with neither vinxi nor SolidJS. Looking at vinxi's code, I'm fairly confident that my issue originated from here, but I may be wrong so please let me know! :)

Note

(probably irrelevant)

  • This was only tested in node (20.11.0), not any other runtime like deno or bun.
  • This was only tested on Winows (10)

TL;DR

Having runtime errors in config file (e.g. app.config.ts) when launching the dev server and then saving the file without fixing errors triggers a watcher change detection, which causes the app to reload, which causes a new configuration file copy to be created and ran, which still throws, which triggers a watching change detection, whis causes the app to reload, (… etc).

Steps to reproduce

  1. Prepare scenario:

    • EITHER create a new SolidStart project and edit the app.config.ts file to add a console.log(__dirname) statement
    • OR simple clone this repro repository
  2. Start the dev server by running pnpm dev (or whatever package manager equivalent):

    • See that an error is thrown because __dirname is not defined in ESM modules.
    • See that the dev comment isn't exiting despite the error
  3. Resave the app.config.ts without fixing errors:

    • This continuously restarts and errors the app
    • And therefore creates a lot of config file copies until the process is killed.

Context

I needed to create a basic SolidStart project to tackle an issue on a Vite plugin of mine and accidentally ran into an issue. I pasted some code in the app.config.ts file of my SolidStart ESM basic project that referenced __dirname . When launching the dev server through pnpm dev —as you'd expect— it caused the config file to throw a ReferenceError (__dirname is not defined, I should have used Node's >= 20's import.meta.dirname instead).

1/ First problem (low severity)

At this point, vinxi created a (I suppose temporary) timestamped copy of my app.config.ts file. That copy was ran and threw the ReferenceError mentioned above. Here's the first question about that issue: Shouldn't there be some kind of mechanism to automatically delete that supposedly temporary timestamped copy of the configuration file whenever the dev server crashes?

Re-running that command keeps creating new copies that are not cleaned up.

my-app/
├─ …
├─ app.config.timestamp_1729965432843.js
├─ app.config.timestamp_1729965436389.js
├─ app.config.timestamp_1729965441762.js
├─ app.config.ts
└─ …

2/ Second problem

When I realized my mistake after the first error occurred, I went to make changes on that app.config.ts file while the CLI was still running (meaning vinxi's chokidar watcher was also still running). And… because I'm an obsessed CTRL+S spammer, I ended up saving a version of the config file that still errored…

1. Saving config changes

Saving that app.config.ts caused the watcher to detect a change, resulting in the app reloading:

vinxi change detected in change // <- what's change? Isn't that supposed to be the config file path?
vinxi reloading app

Which looks completely normal to me for a lot of obvious reasons (a part from that "change" path which is a bit confusing).

2. Reloading the app

When the app reloads, it creates yet another copy of that app.config.ts and runs it, which again —remember I saved an errored version— throws an error and prevents the app to successfully load. The error apparently/somehow causes the watcher to detect changes, which results in—once again— the app reloading;

vinxi change detected in add // <- what's add? Isn't that supposed to be a path?
vinxi reloading app

That seems a little bit weird to me: What file is actually changed because of that error? Is that some kind of log file being caught by the watcher? And why that "add" thing (see third problem)? And most importantly, why should vinxi keep on reloading app indefinitely when an error is caught within the config file copy and that the original config file never changed?

This causes tons of files to be created until the process is killed, which could silently blow up disk usage

my-app/
├─ …
├─ app.config.timestamp_1729965845832.js
├─ app.config.timestamp_1729965847186.js
├─ app.config.timestamp_1729965847195.js
├─ app.config.timestamp_1729965847203.js
├─ (tons of other files…)
├─ app.config.timestamp_1729965849426.js
├─ app.config.timestamp_1729965849445.js
├─ app.config.timestamp_1729965849464.js
├─ app.config.timestamp_1729965849485.js
├─ app.config.ts
└─ …

3/ Third problem (off topic)

I've mentioned the watcher being involved several times. Specifically, there were two watcher-related lines that surprised me:

vinxi change detected in change
vinxi change detected in add

Where are those "change" and "add" values coming from? I'd expect the path of the changed/removed/added file instead? I didn't check but it somehow looks like chokidar event names are being printed instead of paths (again I didn't check yet so I might be wrong).

My two cents

Once again, I'm not familiar with the whole ecosystem so my insight probably isn't worth much, but I naively feel like this could simply be an issue with chokidar's configuration.

The main issue originates from chokidar detecting changes whenever the config file errors and the app consequently reloading, causing that whole mess! If errors on config file didn't cause chokidar to detect changes, then I guess this issue wouldn't occur and we would only stick to problem 1/ which is reasonable.

So my very naive guess would be that finding the origin of that "change detection" and preventing it could help fixing this issue. Unfortunately, I don't know that project so I couldn't legitimately suggest any change. I'd still be happy to help, though! 🤗

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

No branches or pull requests

1 participant