re-initialize watchers on rename

[mbostock]

Ref. #64 (review)

[prichey]

I'm not sure this works as expected. Here's what I just tested:

• Go to http://127.0.0.1:3000/
• Modify docs/index.md, save
• New content appears
• git checkout docs/index.md
• Content at http://127.0.0.1:3000/ does not refresh until a subsequent save, which appears to close / open the socket anyway

Another test case (moving files rather than the git checkout example above):

• Go to http://127.0.0.1:3000/
• Modify docs/index.md, save
• New content appears
• mv docs/index.md docs/index-foo.md
• Dev server crashes:

node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[Error: ENOENT: no such file or directory, open 'docs/index-foo.md'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: 'docs/index-foo.md'
}

I'll defer to your preferences here but my inclination is that location.reload() is (at least now) relatively cheap and handles both situations well. Losing ephemeral page state on mv or git checkout seems reasonable to me, but there may be considerations I'm unaware of here

[mbostock]

Here’s what I see. Are you seeing something different? What operating system and version of Node are you running? I am using macOS 14.0 and Node v18.12.1.

Screen.Recording.2023-10-26.at.7.14.07.AM.mov

Putting aside desired actual outcome for a moment, I don’t see any conceptual reason to force a client reload here: there’s no state on the client that is incorrect that needs to be reloaded (nothing that can’t be addressed with a normal incremental update). The only issue with a file being renamed is that the server ends up watching the wrong (moved) file, and we need the server to watch the file in the original place.

The practical impact of forcing a client reload should just be to introduce a small and unpredictable delay. If we think we need a small delay, we could do that on the server, too. (Though re-initializing the watcher is already a little slow because we haven’t optimized that path.) Furthermore, forcing a client reload is a bad experience because it discards ephemeral client-side state: potentially expensive operations such as fetching data from remote servers and querying databases need to be re-run, interactive inputs are reset to their default state, and the page scrolls to the top. We want to avoid that as much as possible for a good user experience.

Edit: Looking more closely at my video, something appears to be triggering a client reload in this branch? I’ll investigate further as I’m not sure what’s happening.

[mbostock]

Re. mv docs/index.md docs/index-foo.md, I think the server disconnecting is the desired experience since the page being previewed no longer exists. (It shouldn’t crash the server though.)

[prichey]

I'm still on Monterey (12.5.1) but also running Node 18.12.1. Here's what I see:

rename-event.mov

[mbostock]

I’ve spotted the problem here (at least in part — there may be some other issue with file watchers macOS 12 that I won’t know how to fix). By calling refreshMarkdown again, we’re resetting current, which results in us getting out-of-sync with what’s on the client.

I’ll also merge the fixes for #63 recently landed in main.

[mbostock]

Okay, I appear to have got it working correctly now (at least on macOS 14).

Screen.Recording.2023-10-26.at.9.02.42.AM.mov

The latest commits make the following changes:

• Instead of calling refreshMarkdown again, simply trigger a change event following a rename, after re-initializing the file watcher.
• Introduce a brief delay before triggering the change event following a rename, such that we hopefully avoid seeing the file while it is empty, which would blow away ephemeral state on the client. (We should generalize this delay-on-empty technique in the future.)
• Handle ENOENT after a rename event by closing the socket. I don’t think we need to do anything more here, but we don’t want the server to crash.

[mbostock]

Please take another look @prichey @wiltsecarpenter? 🙏

[prichey]

LGTM! Works in all the cases I tested