fix #1113: improve watch mode accuracy (#1676)

CHANGELOG.md

@@ -1,5 +1,17 @@
 # Changelog
 
+## Unreleased
+
+* Improve watch mode accuracy ([#1113](https://github.com/evanw/esbuild/issues/1113))
+
+    Watch mode is enabled by `--watch` and causes esbuild to become a long-running process that automatically rebuilds output files when input files are changed. It's implemented by recording all calls to esbuild's internal file system interface and then invalidating the build whenever these calls would return different values. For example, a call to esbuild's internal `ReadFile()` function is considered to be different if either the presence of the file has changed (e.g. the file didn't exist before but now exists) or the presence of the file stayed the same but the content of the file has changed.
+
+    Previously esbuild's watch mode operated at the `ReadFile()` and `ReadDirectory()` level. When esbuild checked whether a directory entry existed or not (e.g. whether a directory contains a `node_modules` subdirectory or a `package.json` file), it called `ReadDirectory()` which then caused the build to depend on that directory's set of entries. This meant the build would be invalidated even if a new unrelated entry was added or removed, since that still changes the set of entries. This is problematic when using esbuild in environments that constantly create and destroy temporary directory entries in your project directory. In that case, esbuild's watch mode would constantly rebuild as the directory was constantly considered to be dirty.
+
+    With this release, watch mode now operates at the `ReadFile()` and `ReadDirectory().Get()` level. So when esbuild checks whether a directory entry exists or not, the build should now only depend on the presence status for that one directory entry. This should avoid unnecessary rebuilds due to unrelated directory entries being added or removed. The log messages generated using `--watch` will now also mention the specific directory entry whose presence status was changed if a build is invalidated for this reason.
+
+    Note that this optimization does not apply to plugins using the `watchDirs` return value because those paths are only specified at the directory level and do not describe individual directory entries. You can use `watchFiles` or `watchDirs` on the individual entries inside the directory to get a similar effect instead.
+
 ## 0.13.4
 
 * Fix permission issues with the install script ([#1642](https://github.com/evanw/esbuild/issues/1642))

internal/bundler/bundler.go

@@ -697,7 +697,9 @@ func runOnResolvePlugins(
 				fsCache.ReadFile(fs, file)
 			}
 			for _, dir := range result.AbsWatchDirs {
-				fs.ReadDirectory(dir)
+				if entries, err, _ := fs.ReadDirectory(dir); err == nil {
+					entries.SortedKeys()
+				}
 			}
 
 			// Stop now if there was an error
@@ -812,7 +814,9 @@ func runOnLoadPlugins(
 				fsCache.ReadFile(fs, file)
 			}
 			for _, dir := range result.AbsWatchDirs {
-				fs.ReadDirectory(dir)
+				if entries, err, _ := fs.ReadDirectory(dir); err == nil {
+					entries.SortedKeys()
+				}
 			}
 
 			// Stop now if there was an error

internal/fs/fs.go

@@ -3,6 +3,7 @@ package fs
 import (
 	"errors"
 	"os"
+	"sort"
 	"strings"
 	"sync"
 	"syscall"
@@ -44,17 +45,37 @@ func (e *Entry) Symlink(fs FS) string {
 	return e.symlink
 }
 
-type DirEntries struct {
-	dir  string
-	data map[string]*Entry
+type accessedEntries struct {
+	mutex      sync.Mutex
+	wasPresent map[string]bool
+
+	// If this is nil, "SortedKeys()" was not accessed. This means we should
+	// check for whether this directory has changed or not by seeing if any of
+	// the entries in the "wasPresent" map have changed in "present or not"
+	// status, since the only access was to individual entries via "Get()".
+	//
+	// If this is non-nil, "SortedKeys()" was accessed. This means we should
+	// check for whether this directory has changed or not by checking the
+	// "allEntries" array for equality with the existing entries list, since the
+	// code asked for all entries and may have used the presence or absence of
+	// entries in that list.
+	//
+	// The goal of having these two checks is to be as narrow as possible to
+	// avoid unnecessary rebuilds. If only "Get()" is called on a few entries,
+	// then we won't invalidate the build if random unrelated entries are added
+	// or removed. But if "SortedKeys()" is called, we need to invalidate the
+	// build if anything about the set of entries in this directory is changed.
+	allEntries []string
 }
 
-func MakeEmptyDirEntries(dir string) DirEntries {
-	return DirEntries{dir, make(map[string]*Entry)}
+type DirEntries struct {
+	dir             string
+	data            map[string]*Entry
+	accessedEntries *accessedEntries
 }
 
-func (entries DirEntries) Len() int {
-	return len(entries.data)
+func MakeEmptyDirEntries(dir string) DirEntries {
+	return DirEntries{dir, make(map[string]*Entry), nil}
 }
 
 type DifferentCase struct {
@@ -65,7 +86,17 @@ type DifferentCase struct {
 
 func (entries DirEntries) Get(query string) (*Entry, *DifferentCase) {
 	if entries.data != nil {
-		if entry := entries.data[strings.ToLower(query)]; entry != nil {
+		key := strings.ToLower(query)
+		entry := entries.data[key]
+
+		// Track whether this specific entry was present or absent for watch mode
+		if accessed := entries.accessedEntries; accessed != nil {
+			accessed.mutex.Lock()
+			accessed.wasPresent[key] = entry != nil
+			accessed.mutex.Unlock()
+		}
+
+		if entry != nil {
 			if entry.base != query {
 				return entry, &DifferentCase{
 					Dir:    entries.dir,
@@ -76,16 +107,28 @@ func (entries DirEntries) Get(query string) (*Entry, *DifferentCase) {
 			return entry, nil
 		}
 	}
+
 	return nil, nil
 }
 
-func (entries DirEntries) UnorderedKeys() (keys []string) {
+func (entries DirEntries) SortedKeys() (keys []string) {
 	if entries.data != nil {
 		keys = make([]string, 0, len(entries.data))
 		for _, entry := range entries.data {
 			keys = append(keys, entry.base)
 		}
+		sort.Strings(keys)
+
+		// Track the exact set of all entries for watch mode
+		if entries.accessedEntries != nil {
+			entries.accessedEntries.mutex.Lock()
+			entries.accessedEntries.allEntries = keys
+			entries.accessedEntries.mutex.Unlock()
+		}
+
+		return keys
 	}
+
 	return
 }
 
@@ -152,8 +195,11 @@ type FS interface {
 }
 
 type WatchData struct {
-	// These functions return true if the file system entry has been modified
-	Paths map[string]func() bool
+	// These functions return a non-empty path as a string if the file system
+	// entry has been modified. For files, the returned path is the same as the
+	// file path. For directories, the returned path is either the directory
+	// itself or a file in the directory that was changed.
+	Paths map[string]func() string
 }
 
 type ModKey struct {

internal/fs/fs_mock.go

@@ -29,7 +29,7 @@ func MockFS(input map[string]string) FS {
 			kDir := path.Dir(k)
 			dir, ok := dirs[kDir]
 			if !ok {
-				dir = DirEntries{kDir, make(map[string]*Entry)}
+				dir = DirEntries{kDir, make(map[string]*Entry), nil}
 				dirs[kDir] = dir
 			}
 			if kDir == k {

internal/fs/fs_real.go

@@ -38,20 +38,20 @@ type entriesOrErr struct {
 type watchState uint8
 
 const (
-	stateNone               watchState = iota
-	stateDirHasEntries                 // Compare "dirEntries"
-	stateDirMissing                    // Compare directory presence
-	stateFileHasModKey                 // Compare "modKey"
-	stateFileNeedModKey                // Need to transition to "stateFileHasModKey" or "stateFileUnusableModKey" before "WatchData()" returns
-	stateFileMissing                   // Compare file presence
-	stateFileUnusableModKey            // Compare "fileContents"
+	stateNone                  watchState = iota
+	stateDirHasAccessedEntries            // Compare "accessedEntries"
+	stateDirMissing                       // Compare directory presence
+	stateFileHasModKey                    // Compare "modKey"
+	stateFileNeedModKey                   // Need to transition to "stateFileHasModKey" or "stateFileUnusableModKey" before "WatchData()" returns
+	stateFileMissing                      // Compare file presence
+	stateFileUnusableModKey               // Compare "fileContents"
 )
 
 type privateWatchData struct {
-	dirEntries   []string
-	fileContents string
-	modKey       ModKey
-	state        watchState
+	accessedEntries *accessedEntries
+	fileContents    string
+	modKey          ModKey
+	state           watchState
 }
 
 type RealFSOptions struct {
@@ -133,7 +133,7 @@ func (fs *realFS) ReadDirectory(dir string) (entries DirEntries, canonicalError
 
 	// Cache miss: read the directory entries
 	names, canonicalError, originalError := fs.readdir(dir)
-	entries = DirEntries{dir, make(map[string]*Entry)}
+	entries = DirEntries{dir, make(map[string]*Entry), nil}
 
 	// Unwrap to get the underlying error
 	if pathErr, ok := canonicalError.(*os.PathError); ok {
@@ -157,14 +157,14 @@ func (fs *realFS) ReadDirectory(dir string) (entries DirEntries, canonicalError
 	if fs.watchData != nil {
 		defer fs.watchMutex.Unlock()
 		fs.watchMutex.Lock()
-		state := stateDirHasEntries
+		state := stateDirHasAccessedEntries
 		if canonicalError != nil {
 			state = stateDirMissing
 		}
-		sort.Strings(names)
+		entries.accessedEntries = &accessedEntries{wasPresent: make(map[string]bool)}
 		fs.watchData[dir] = privateWatchData{
-			dirEntries: names,
-			state:      state,
+			accessedEntries: entries.accessedEntries,
+			state:           state,
 		}
 	}
 
@@ -432,7 +432,7 @@ func (fs *realFS) kind(dir string, base string) (symlink string, kind EntryKind)
 }
 
 func (fs *realFS) WatchData() WatchData {
-	paths := make(map[string]func() bool)
+	paths := make(map[string]func() string)
 
 	for path, data := range fs.watchData {
 		// Each closure below needs its own copy of these loop variables
@@ -454,42 +454,70 @@ func (fs *realFS) WatchData() WatchData {
 
 		switch data.state {
 		case stateDirMissing:
-			paths[path] = func() bool {
+			paths[path] = func() string {
 				info, err := os.Stat(path)
-				return err == nil && info.IsDir()
+				if err == nil && info.IsDir() {
+					return path
+				}
+				return ""
 			}
 
-		case stateDirHasEntries:
-			paths[path] = func() bool {
+		case stateDirHasAccessedEntries:
+			paths[path] = func() string {
 				names, err, _ := fs.readdir(path)
-				if err != nil || len(names) != len(data.dirEntries) {
-					return true
+				if err != nil {
+					return path
 				}
-				sort.Strings(names)
-				for i, s := range names {
-					if s != data.dirEntries[i] {
-						return true
+				data.accessedEntries.mutex.Lock()
+				defer data.accessedEntries.mutex.Unlock()
+				if allEntries := data.accessedEntries.allEntries; allEntries != nil {
+					// Check all entries
+					if len(names) != len(allEntries) {
+						return path
+					}
+					sort.Strings(names)
+					for i, s := range names {
+						if s != allEntries[i] {
+							return path
+						}
+					}
+				} else {
+					// Check individual entries
+					isPresent := make(map[string]bool, len(names))
+					for _, name := range names {
+						isPresent[strings.ToLower(name)] = true
+					}
+					for name, wasPresent := range data.accessedEntries.wasPresent {
+						if wasPresent != isPresent[name] {
+							return fs.Join(path, name)
+						}
 					}
 				}
-				return false
+				return ""
 			}
 
 		case stateFileMissing:
-			paths[path] = func() bool {
-				info, err := os.Stat(path)
-				return err == nil && !info.IsDir()
+			paths[path] = func() string {
+				if info, err := os.Stat(path); err == nil && !info.IsDir() {
+					return path
+				}
+				return ""
 			}
 
 		case stateFileHasModKey:
-			paths[path] = func() bool {
-				key, err := modKey(path)
-				return err != nil || key != data.modKey
+			paths[path] = func() string {
+				if key, err := modKey(path); err != nil || key != data.modKey {
+					return path
+				}
+				return ""
 			}
 
 		case stateFileUnusableModKey:
-			paths[path] = func() bool {
-				buffer, err := ioutil.ReadFile(path)
-				return err != nil || string(buffer) != data.fileContents
+			paths[path] = func() string {
+				if buffer, err := ioutil.ReadFile(path); err != nil || string(buffer) != data.fileContents {
+					return path
+				}
+				return ""
 			}
 		}
 	}

internal/resolver/resolver.go

@@ -796,7 +796,7 @@ func (r resolverQuery) dirInfoCached(path string) *dirInfo {
 		if cached == nil {
 			r.debugLogs.addNote(fmt.Sprintf("Failed to read directory %q", path))
 		} else {
-			count := cached.entries.Len()
+			count := len(cached.entries.SortedKeys())
 			entries := "entries"
 			if count == 1 {
 				entries = "entry"

pkg/api/api_impl.go

@@ -1212,11 +1212,11 @@ func (w *watcher) tryToFindDirtyPath() string {
 
 	// Always check all recent items every iteration
 	for i, path := range w.recentItems {
-		if w.data.Paths[path]() {
+		if dirtyPath := w.data.Paths[path](); dirtyPath != "" {
 			// Move this path to the back of the list (i.e. the "most recent" position)
 			copy(w.recentItems[i:], w.recentItems[i+1:])
 			w.recentItems[len(w.recentItems)-1] = path
-			return path
+			return dirtyPath
 		}
 	}
 
@@ -1230,15 +1230,15 @@ func (w *watcher) tryToFindDirtyPath() string {
 
 	// Check if any of the entries in this iteration have been modified
 	for _, path := range toCheck {
-		if w.data.Paths[path]() {
+		if dirtyPath := w.data.Paths[path](); dirtyPath != "" {
 			// Mark this item as recent by adding it to the back of the list
 			w.recentItems = append(w.recentItems, path)
 			if len(w.recentItems) > maxRecentItemCount {
 				// Remove items from the front of the list when we hit the limit
 				copy(w.recentItems, w.recentItems[1:])
 				w.recentItems = w.recentItems[:maxRecentItemCount]
 			}
-			return path
+			return dirtyPath
 		}
 	}
 	return ""

pkg/api/serve_other.go

@@ -193,7 +193,7 @@ func (h *apiHandler) ServeHTTP(res http.ResponseWriter, req *http.Request) {
 		if h.servedir != "" && kind != fs.FileEntry {
 			if entries, err, _ := h.fs.ReadDirectory(h.fs.Join(h.servedir, queryPath)); err == nil {
 				kind = fs.DirEntry
-				for _, name := range entries.UnorderedKeys() {
+				for _, name := range entries.SortedKeys() {
 					entry, _ := entries.Get(name)
 					switch entry.Kind(h.fs) {
 					case fs.DirEntry: