[Bug?] 'watchFiles' does not appear to be watched after errors

[zaydek]

I’m experimenting with the new watchFiles API that was included in 0.10.0 and noticed that when a plugin throws, watchFiles are no longer being watched. It’s a little hard to put into words so I recorded a small video.

Essentially, after I syntactically break Sass and save, I see watch.onRebuild fires, but then further saves (where the source Sass file actually changes, but is still syntactically broken), further watch.onRebuild events do not fire.

Minimal repro here: https://github.com/zaydek/esbuild-watchfiles-bug.

Edit: If the video doesn’t appear for any reason, you can download it from here.

https://user-images.githubusercontent.com/58870766/112622638-a6bec900-8e6e-11eb-8039-8bfe8f6a14d7.mov

[evanw]

What would be the desired behavior here? Falling back to the previous watchFiles value would fix this case, although I suspect doing that isn't fully correct. For example, what happens when the first run of the plugin fails?

The fundamental issue here is that watchFiles should ideally include all observed files, even in the case where there's an error. This isn't an issue for esbuild itself because esbuild still accumulates information in error scenarios. Another option could be to entirely fall back to the set of watch paths from the previous build when there's a build error, but that doesn't work if the first build fails.

FWIW the plugin should be able to handle this itself by using try/catch and returning {errors, watchFiles}. It'd still be great for esbuild to do something more helpful in this case of course.

Random thought: I wonder if an API change could help. For example, maybe instead of returning watchFiles and watchDirs you could have to call args.watchFile() and args.watchDir() and then esbuild could keep that information even if an exception is thrown later.

[zaydek]

What would be the desired behavior here? Falling back to the previous watchFiles value would fix this case, although I suspect doing that isn't fully correct. For example, what happens when the first run of the plugin fails?

I think #1037 is probably the right philosophy. I think there is an expectation that when watch mode is enabled, whether or not a build is successful, esbuild should act like a persistent dev server. So specifically, when watch mode is enabled, I think it makes more sense for esbuild to never crash unless there’s some kind of fatal condition (like panic("Internal error")).

but that doesn't work if the first build fails.

Ah, that makes sense. How can you watch when you don’t know what to watch? I hadn’t thought about that condition.

FWIW the plugin should be able to handle this itself by using try/catch and returning {errors, watchFiles}. It'd still be great for esbuild to do something more helpful in this case of course.

This makes sense on its face but I think it’s not necessarily the right design simply because every plugin author needs to implement their own error-handling. That seems like a good idea, and I even think with a language like Go this is very reasonable and sort of the expectation, but I don’t think that will happen most of the time with JS / node-based plugins. At least it’d be nice -- from a user’s perspective -- if I can throw together Sass and MDX plugins and not worry about how Sass or MDX handles errors, and just leave it up to the stack trace that gets reported back to me by esbuild.

I know this design is a little dirty but I think this is somewhat inevitable with JS / node-based error-handling. Of course, this is just my opinion, but there are so many ways JS / node programs can error even when they are typed (e.g TypeScript). There are thrown errors, callback errors, uncaughtException, errors from code not being await-ed on, etc. I don’t think every plugin author (for JS / node) will want to think about these errors states. Again, I’m only speaking to JS / node here. Go figured out from the beginning that error observability is a good thing and is basically a solved problem.

I’m not against returning errors for JS / node-based plugins, and if that’s already supported I didn’t know about it. Would there be operational differences between that vs. letting plugins throw on errors?

Random thought: I wonder if an API change could help. For example, maybe instead of returning watchFiles and watchDirs you could have to call args.watchFile() and args.watchDir() and then esbuild could keep that information even if an exception is thrown later.

It sounds like the difference here is that his gives plugins a chance to be preemptive about which files / directories are to be watched versus gathering this info from the return statement, which may be iffy because of unhandled errors / exceptions. It makes sense that you would want to do this but with Sass at least, I believe I only get access to includedFiles on the returned result from Sass and I don’t really know this in advance (at the start of onResolve) because other than args.path, linked Sass files are opaque to me as a user. Sass resolves them for me and returns an array I can pass to esbuild.

So I think if and error occurs during watch mode, esbuild should still watch args.path even if it doesn’t know the linked dependencies yet. I know what you mean about this not being ideal because esbuild won’t pick up linked dependencies. Would it not make more sense for esbuild to just watch all .scss files in this case? Although I suppose this doesn’t work cleanly because .scss files can import .css files, and those would not be picked up by greedily watching .scss extensions.

FWIW, the typed Sass result looks like this (from Sass’s `index.d.ts`):

```ts export interface Result { /** * The compiled CSS. * * Write this to a file, or serve it out as needed. */ css: Buffer; /** * The source map. */ map?: Buffer; stats: { /** * The path to the scss file, or `data` if the source was not a file. */ entry: string; /** * `Date.now()` before the compilation. */ start: number; /** * `Date.now()` after the compilation. */ end: number; /** * `end - start` */ duration: number; /** * Absolute paths to all related files in no particular order. */ includedFiles: string[]; }; } ```

---

These are some more thoughts that are less organized but I think will shed light on my experience with esbuild’s watch features in general. These are meant to suffice simply as constructive criticism and as an honest accounting of my experience as a consumer of esbuild.

Having worked with esbuild and the watch features for a little while, my experience can be summed up as this: while the watch features are convenient and ideally engineered, they are less ergonomic than simply implementing your own watcher implementation. Therefore I’ve found using `incremental: true` and `result.rebuild` to be far more predictable than leveraging esbuild’s native watch features. I’ll do my best to explain why. If you’re curious about my real-world use case, [the relevant esbuild code is here](https://github.com/zaydek/retro/blob/609d76726c73077d3e20fa91a8d8c3a02583c6a8/scripts/backend.ts). Essentially what I’m doing is using Go as the parent process and creating an IPC service that uses JSON to communicate with node over stdio. [That is implemented here](https://github.com/zaydek/retro/blob/609d76726c73077d3e20fa91a8d8c3a02583c6a8/pkg/ipc/ipc.go). That simply gives me channels to pipe in stdin and get back stdout or stderr. I’m using stdin to send commands and stdout to get build responses from esbuild, specifically errors and warnings messages that I use the new formatter API to format and report back to the user when a build breaks. stderr is reserved for operational errors and are treated as exceptions to Go (e.g. will panic). OK so now that that’s out of the way, I need to be able to predictably trigger rebuilds on file changes. Theoretically I can use esbuild for this but the problems are the following: - With esbuild watch, plugins need to be ‘watch mode aware‘. This means plugins need to additionally return `watchFiles` and `watchDirs`, and if I’m not mistaken, if I don’t do this step, plugins _won’t_ be watched because they are virtual. Ideally I edit a file and I see a change, for example from an `import "App.scss"` statement. With `incremental: true` and `result.rebuild`, these seems to not be a problem -- Sass files rebuild as desired (even if I omit `watchFiles` and `watchDirs`). - esbuild’s watch is slightly less predictable. By this I mean that I break the build, save, and then make another breaking save (e.g. two syntactical errors), esbuild seems to not report to stderr a second time. This is the cause for the OP of this thread, which I hope I captured in the video / repro. - I’ve been confused previously about `result.rebuild` triggering the `watch.onRebuild` callback. I understand now that these are separate APIs, and I even think that’s a good thing because as you said, the APIs are orthogonal and therefore can be composed. What I didn’t like about this was needing to repeat the same logic in the scope of the `watch.onRebuild` callback and again with a `try-catch` block around `result.rebuild`. This felt weird to me because it wasn’t actually identical code: the `watch.onRebuild` callback accepts a result and error parameters whereas with `try-catch` I need to interrogate the return of result `result.warnings` and inspect the thrown error’s `error.warnings and error.errors`. I’m not complaining about this -- I just want to point out that I can’t easily reuse the same logic between `watch.onRebuild` and `try { result.rebuild() }` because results and errors are handled / returned differently. The point is that I’ve found using esbuild’s watch features to not be composable. Alternatively, by simply using `incremental: true` and `result.rebuild`, everything is working almost perfectly and there are many, many less surprises or confusing states to reason about. I think esbuild very much benefits from having its own watch mode, but for what I’m building where I need more predictability / composability, it’s the wrong API for my use-case. I should mention that [this is how and where I tell esbuild to programmatically rebuild from Go](https://github.com/zaydek/retro/blob/609d76726c73077d3e20fa91a8d8c3a02583c6a8/cmd/retro/retro.go#L133). My watch mode is not CPU optimized yet but it does make my life easy because I have so much less to worry about by simply greedily watching `src`. If something outside of `src` changes, it takes less than esbuild to start over so it’s not that big of a deal to me to occasionally restart the dev server as needed.

[glromeo]

As a esbuild user I find myself sharing the same experience of @zaydek on watch vs incremental so I prefer to avoid watch mode altogether but, as plugin developer, I can see people prefer a pre-packaged solution so I am doing my best to make it work.

@evanw I think that #1037 is a reasonable expectation and despite for now I fixed this issue by returning {errors, watchedFiles} this way I have to keep a cache of all the watched files in the plugin.

What I would like is that esbuild, which already has this cache, leaves it unmodified until the next successful build so that errors can be fixed and the change detected by the last successful build watched files if that makes sense.