Strengthen and idiot-proof the `watchPath` contract

[savetheclocktower]

Have you checked for existing feature requests?

• Completed

Summary

The idiot described is hypothetical — but describes all of us. Including me!

What’s the problem?

Our exported watchPath function is a footgun.

It allows community packages to set up file watchers with an API that Pulsar provides. The rationale is that it’s better to have them using our provided API than for each package to use its own solution; at least this way we can cut down on redundancies and spare those packages from including libraries likely to have native module dependencies.

But it’s pretty hard to use responsibly:

• You can pass it the path to a single file and get somewhat rational behavior, but…
• if you call it with a path to an arbitrary folder on disk — its primary use, if you believe the documentation — then you’ll create a recursive watcher.
• Recursively watching a folder has a cost that varies across platforms. On macOS it’s rather cheap thanks to FSEvents, but on Linux it’s quite costly. Each individual folder needs its own inotify watch, of which there are only a fixed number allowed to be created by the OS.

How should we fix it?

• We must consider core.ignoredNames when creating any folder watcher, whether the destination is within the project or outside of it. This is a common-sense solution that will get us 95% of what we need.
• We should decide whether to keep Pulsar’s complex system that tries to consolidate and reuse recursive watchers. I’m sure it has upsides, but it also prevents any customization of inclusions or exclusions on the consumer’s side, since you might ask to watch /foo/bar/baz and have an existing watcher on /foo/bar reused (making it harder to apply your own exclusions, since you don’t own that watcher!). You can always wait until receiving events to apply exclusion filtering, but this doesn't help with the goal of preventing unneeded inotify watchers on Linux.

How does this affect the watchPath contract?

On the Atom/Pulsar side, watchPath was also designed to allow for modularity of watcher implementations. The Atom folks weren’t locked into one particular watcher and were even developing their own for a while.

So if we change the theoretical behavior of watchPath, we should also make sure our current implementation (and other potential implementations, like @parcel/watcher) can support it.

Here are some proposed additions that should be rather easy for an implementation to handle:

1. watchPath should be able to accept an arbitrary path to a file (rather than a folder) and handle it by attaching a non-recursive watcher to that single file. This works (as mentioned above), but I don’t think there’s any documentation that covers it; so the fact that it works in both nsfw and @parcel/watcher is accidental. But it’s useful and helps cut down on costly filewatching behavior.
2. Any recursive file-watching strategy must exclude any directories mentioned in our core.ignoredNames setting. Some of these will be files and some will be folders; the important part isn’t to ignore events on the files, but instead to prevent inotify watches from being added on (or beneath) the ignored directories.

Here’s one I’d love to add, but which seems strangely omitted from both candidate implementations:

• Non-recursive folder watching would ideally be possible. It’s possible now via the Directory class (which delegates to the older pathwatcher library), but it doesn’t make a lot of sense that we must use one API for recursive directory watching and another for non-recursive directory watching.

How can this coexist with watcher reuse?

The two additions I described above should be easy to add without affecting watcher reuse. Since all native watchers would be operating under the same list of ignored names, there wouldn’t be any chance of conflict.

If we ever did add the ability to customize excludes, we’d have to do it one of two ways:

• Any exclusions that go past the list in core.ignoredNames would have to be implemented via filtering, rather than by narrowing what the native watcher listens to. (Since a given call to watchPath may create a new native watcher or reuse an existing one, the PathWatcher class would need to be the one consulting that list to decide if an event should be ignored.)
• Alternatively, if we really wanted the native watcher’s filtering to be customizable, we could abandon watcher reuse — either entirely or for the watchers that have asked for customized exclusions.

What benefits does this feature provide?

Linux users should be much, much less likely to run up against inotify limits; they'll therefore enjoy more reliable file-watching behavior.

Any alternatives?

I can't think of any alternatives that aren't more disruptive to the existing watchPath contract. We want to fix this API as much as we can without affecting legitimate and common use cases among community packages.

Other examples:

No response

[savetheclocktower]

#1299 implements this, but that's an experimental PR for PulsarNext.

Annoyingly, nsfw doesn't allow you to define excluded paths via glob; you've got to convert them to a comprehensive list of actual paths to exclude. It takes a lot of work to do that, especially if it's happening on the renderer process.

#1299 fixes this by moving the nsfw watcher into a separate process, just as it does for @parcel/watcher (which does accept exclusions via glob, luckily). That allows it to do the necessary crawling in the fastest possible way (whether sync or async) without locking up the renderer. But that's a big change and I wouldn't necessarily mind if we put it off until after the Electron upgrade. (That upgrade will also move us from Node 14 to Node 20, perhaps improving the performance of the filesystem crawl in the first place.)