From 0a4ae7730d46806c8cd71b4b96d8c3502602de6c Mon Sep 17 00:00:00 2001
From: Imran Khan <imrankhan@teknik.io>
Date: Thu, 3 Nov 2022 19:00:38 +0600
Subject: [PATCH] Fix not rebuilding files when `rename` event is emit (#9689)

* Fix CLI not rebuilding files when `rename` event is emit

* Refactor watching code

* Simplify

* Add rebuild timer

* Move timer into `recordChangedFile`

Co-authored-by: Jordan Pittman <jordan@cryptica.me>
---
 src/cli/build/watching.js | 112 ++++++++++++++++++++++++++++++++++----
 1 file changed, 101 insertions(+), 11 deletions(-)

diff --git a/src/cli/build/watching.js b/src/cli/build/watching.js
index 9b5462a244fd..c5204b610e54 100644
--- a/src/cli/build/watching.js
+++ b/src/cli/build/watching.js
@@ -8,6 +8,14 @@ import path from 'path'
 
 import { readFileWithRetries } from './utils.js'
 
+/**
+ * The core idea of this watcher is:
+ * 1. Whenever a file is added, changed, or renamed we queue a rebuild
+ * 2. Perform as few rebuilds as possible by batching them together
+ * 3. Coalesce events that happen in quick succession to avoid unnecessary rebuilds
+ * 4. Ensure another rebuild happens _if_ changed while a rebuild is in progress
+ */
+
 /**
  *
  * @param {*} args
@@ -42,27 +50,93 @@ export function createWatcher(args, { state, rebuild }) {
       : false,
   })
 
+  // A queue of rebuilds, file reads, etc… to run
   let chain = Promise.resolve()
-  let pendingRebuilds = new Set()
+
+  /**
+   * A list of files that have been changed since the last rebuild
+   *
+   * @type {{file: string, content: () => Promise<string>, extension: string}[]}
+   */
   let changedContent = []
 
+  /**
+   * A list of files for which a rebuild has already been queued.
+   * This is used to prevent duplicate rebuilds when multiple events are fired for the same file.
+   * The rebuilt file is cleared from this list when it's associated rebuild has _started_
+   * This is because if the file is changed during a rebuild it won't trigger a new rebuild which it should
+   **/
+  let pendingRebuilds = new Set()
+
+  let _timer
+  let _reject
+
+  /**
+   * Rebuilds the changed files and resolves when the rebuild is
+   * complete regardless of whether it was successful or not
+   */
+  async function rebuildAndContinue() {
+    let changes = changedContent.splice(0)
+
+    // There are no changes to rebuild so we can just do nothing
+    if (changes.length === 0) {
+      return Promise.resolve()
+    }
+
+    // Clear all pending rebuilds for the about-to-be-built files
+    changes.forEach((change) => pendingRebuilds.delete(change.file))
+
+    // Resolve the promise even when the rebuild fails
+    return rebuild(changes).then(
+      () => {},
+      () => {}
+    )
+  }
+
   /**
    *
    * @param {*} file
    * @param {(() => Promise<string>) | null} content
+   * @returns {Promise<void>}
    */
   function recordChangedFile(file, content = null) {
     file = path.resolve(file)
 
-    content = content ?? (async () => await fs.promises.readFile(file, 'utf8'))
+    // Applications like Vim/Neovim fire both rename and change events in succession for atomic writes
+    // In that case rebuild has already been queued by rename, so can be skipped in change
+    if (pendingRebuilds.has(file)) {
+      return Promise.resolve()
+    }
+
+    // Mark that a rebuild of this file is going to happen
+    // It MUST happen synchronously before the rebuild is queued for this to be effective
+    pendingRebuilds.add(file)
 
     changedContent.push({
       file,
-      content,
+      content: content ?? (() => fs.promises.readFile(file, 'utf8')),
       extension: path.extname(file).slice(1),
     })
 
-    chain = chain.then(() => rebuild(changedContent.splice(0)))
+    if (_timer) {
+      clearTimeout(_timer)
+      _reject()
+    }
+
+    // If a rebuild is already in progress we don't want to start another one until the 10ms timer has expired
+    chain = chain.then(
+      () =>
+        new Promise((resolve, reject) => {
+          _timer = setTimeout(resolve, 10)
+          _reject = reject
+        })
+    )
+
+    // Resolves once this file has been rebuilt (or the rebuild for this file has failed)
+    // This queues as many rebuilds as there are changed files
+    // But those rebuilds happen after some delay
+    // And will immediately resolve if there are no changes
+    chain = chain.then(rebuildAndContinue, rebuildAndContinue)
 
     return chain
   }
@@ -107,18 +181,34 @@ export function createWatcher(args, { state, rebuild }) {
       return
     }
 
+    // We'll go ahead and add the file to the pending rebuilds list here
+    // It'll be removed when the rebuild starts unless the read fails
+    // which will be taken care of as well
     pendingRebuilds.add(filePath)
 
-    chain = chain.then(async () => {
-      let content
-
+    async function enqueue() {
       try {
-        content = await readFileWithRetries(path.resolve(filePath))
-      } finally {
-        pendingRebuilds.delete(filePath)
+        // We need to read the file as early as possible outside of the chain
+        // because it may be gone by the time we get to it. doing the read
+        // immediately increases the chance that the file is still there
+        let content = await readFileWithRetries(path.resolve(filePath))
+
+        if (content === undefined) {
+          return
+        }
+
+        // This will push the rebuild onto the chain
+        // @ts-ignore: TypeScript isn't picking up that content is a string here
+        await recordChangedFile(filePath, () => content)
+      } catch {
+        // If reading the file fails, it's was probably a deleted temporary file
+        // So we can ignore it and no rebuild is needed
       }
+    }
 
-      return recordChangedFile(filePath, () => content)
+    enqueue().then(() => {
+      // If the file read fails we still need to make sure the file isn't stuck in the pending rebuilds list
+      pendingRebuilds.delete(filePath)
     })
   })