Repository: Shopify/themekit (go)

Issue:
Large CPU consumption with theme watch
**Describe the bug**
When running theme watch for a project with the following ignore_files setting in config.yml (no other settings aside from essential)..

```yaml
  ignore_files:
    - config/*
    - notes/*
    - node_modules/*
    - ./package.json
    - ./package.lock.json
    - prepros*
    - ./.vscode/*
    - liquid-testing/*
    - local-files/*
    - ./README.md
    - tests/*
    - src/*
    - webpack*.js
    - ./tsconfig.json
```
CPU usage starts and remains at at-least 15% - remaining between here and 21% - ish. I am using a fairly fast machine - see specs below.

Is is worth noting that the total project folder size is 141MB. 128MB belongs in the node_modules folder (which is ignored), and a lot of the remaining 13MB is also ignored in the other ignored directories. This only leaves a few MB of project files left that are being watched.

**To Reproduce**
Steps to reproduce the behavior:
1. Run command 'theme watch'.

**Expected behavior**
A low CPU usage, as is typically reported.

**Environment (please complete the following information):**
 - OS [e.g. iOS]: WIndows 10 (latest developers build)
- Themekit version [e.g. v0.7.5]: 1.0.1
 - Editor [e.g. atom, sublime]: VS Code 1.38.1
 - Machine Specs
   - Processor: Processor	Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz, 1992 Mhz, 4 Core(s), 8 Logical Processor(s)
   - RAM: 16GB DDR4 2200MHz



Pull request:
CPU usage optimization in watch command
fixes #664

In previous versions, I watched each individual project directory however that created problems with creating new directories while the command run, and they were not tracked. Next step I took was to just watch all recursively and filter the events to just the project directories. This worked except, those with node_modules folders found that this caused their laptop to melt. 

This version now watched the root directory and project directories and does not filter out directory events. Now when a new directory is created, it will catch that event and start watching it, any deleted directories are discarded.

This should clear up the CPU issues.

### Bonus
I updated the dependency which allowed me to not have to parse new and old paths, and I am happy whenever I get to delete string parsing code.

### Warn Checklist
- [ ] This changes the interface and requires a Major/Minor version change.
- [x] I have :tophat:'d these changes by using the commands I changed by hand.
- [ ] I have added a dependancy to the project.
- [x] I have considered any potential impact on [node-themekit](https://github.com/Shopify/node-themekit)


Patch:
diff --git a/changelog.txt b/changelog.txt
index f362b7d..5783352 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -2,6 +2,7 @@ v1.1.5
 ======
 - Fix layout after renaming application.scss.css to application.css
 - Added more to customer templates
+- Optimization for watcher to reduce CPU usage.
 
 v1.1.4 (Dec 3, 2020)
 ====================
diff --git a/go.mod b/go.mod
index 26c3e7d..df06a45 100644
--- a/go.mod
+++ b/go.mod
@@ -13,7 +13,7 @@ require (
 	github.com/joho/godotenv v1.3.0
 	github.com/mattn/go-colorable v0.0.0-20180310133214-efa589957cd0
 	github.com/mattn/go-isatty v0.0.4 // indirect
-	github.com/radovskyb/watcher v1.0.6
+	github.com/radovskyb/watcher v1.0.7
 	github.com/ryanuber/go-glob v0.0.0-20160226084822-572520ed46db
 	github.com/shibukawa/configdir v0.0.0-20170330084843-e180dbdc8da0
 	github.com/skratchdot/open-golang v0.0.0-20160302144031-75fb7ed4208c
diff --git a/go.sum b/go.sum
index 68aea13..22930ad 100644
--- a/go.sum
+++ b/go.sum
@@ -34,6 +34,8 @@ github.com/radovskyb/watcher v1.0.2 h1:9L5TsZUbo1nKhQEQPtICVc+x9UZQ6VPdBepLHyGw/
 github.com/radovskyb/watcher v1.0.2/go.mod h1:78okwvY5wPdzcb1UYnip1pvrZNIVEIh/Cm+ZuvsUYIg=
 github.com/radovskyb/watcher v1.0.6 h1:8WIQ9UxEYMZjem1OwU7dVH94DXXk9mAIE1i8eqHD+IY=
 github.com/radovskyb/watcher v1.0.6/go.mod h1:78okwvY5wPdzcb1UYnip1pvrZNIVEIh/Cm+ZuvsUYIg=
+github.com/radovskyb/watcher v1.0.7 h1:AYePLih6dpmS32vlHfhCeli8127LzkIgwJGcwwe8tUE=
+github.com/radovskyb/watcher v1.0.7/go.mod h1:78okwvY5wPdzcb1UYnip1pvrZNIVEIh/Cm+ZuvsUYIg=
 github.com/ryanuber/go-glob v0.0.0-20160226084822-572520ed46db h1:ge9atzKq16843f793fDVxKUhmTb4H5muzjJQ6PgsnHg=
 github.com/ryanuber/go-glob v0.0.0-20160226084822-572520ed46db/go.mod h1:807d1WSdnB0XRJzKNil9Om6lcp/3a0v4qIHxIXzX/Yc=
 github.com/shibukawa/configdir v0.0.0-20170330084843-e180dbdc8da0 h1:Xuk8ma/ibJ1fOy4Ee11vHhUFHQNpHhrBneOCNHVXS5w=
diff --git a/src/file/_testdata/project/assets/application.js.liquid b/src/file/_testdata/project/assets/application.js.liquid
new file mode 100644
index 0000000..e69de29
diff --git a/src/file/_testdata/project/javascripts/app.js b/src/file/_testdata/project/javascripts/app.js
new file mode 100644
index 0000000..e69de29
diff --git a/src/file/watcher.go b/src/file/watcher.go
index 7b057c9..a96d3c6 100644
--- a/src/file/watcher.go
+++ b/src/file/watcher.go
@@ -6,7 +6,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"strings"
 	"time"
 
 	"github.com/Shopify/themekit/src/env"
@@ -25,7 +24,6 @@ const (
 	Skip
 	// Get is when a file should be re-fetched, used in download operations
 	Get
-	filepathSplit = " -> "
 )
 
 var (
@@ -67,9 +65,15 @@ func NewWatcher(e *env.Env, configPath string, checksums map[string]string) (*Wa
 	}
 	fsWatcher.AddFilterHook(hook)
 
-	if err := fsWatcher.AddRecursive(e.Directory); err != nil {
+	if err := fsWatcher.Add(e.Directory); err != nil {
 		return nil, fmt.Errorf("Could not watch directory: %s", err)
 	}
+	for _, folder := range assetLocations {
+		path := filepath.Join(e.Directory, folder)
+		if err := fsWatcher.Add(path); err != nil && !os.IsNotExist(err) {
+			return nil, fmt.Errorf("Could not watch directory %s: %s", path, err)
+		}
+	}
 
 	return &Watcher{
 		Events:    make(chan Event),
@@ -85,7 +89,7 @@ func filterHook(e *env.Env, configPath string) (watcher.FilterFileHookFunc, erro
 		return nil, err
 	}
 	return func(info os.FileInfo, fullPath string) error {
-		if info.IsDir() || (configPath != fullPath && filter.Match(fullPath)) {
+		if configPath != fullPath && filter.Match(fullPath) {
 			return watcher.ErrSkip
 		}
 		return nil
@@ -109,11 +113,21 @@ func (w *Watcher) watchFsEvents() {
 		case <-w.fsWatcher.Closed:
 			w.Stop()
 			return
+		case <-w.fsWatcher.Error:
+			// discard errors, they are not useful for users and the watcher deadlocks
+			// if they are not read. The expected errors come from a directory being
+			// deleted while being watched
 		}
 	}
 }
 
 func (w *Watcher) onEvent(event watcher.Event) bool {
+	if event.IsDir() {
+		if isEventType(event.Op, watcher.Create) {
+			w.fsWatcher.Add(event.Path)
+		}
+		return false
+	}
 	events := map[string]Event{}
 	for _, event := range w.translateEvent(event) {
 		events[event.Path] = event
@@ -149,7 +163,7 @@ func (w *Watcher) updateChecksum(e Event) {
 }
 
 func (w *Watcher) translateEvent(event watcher.Event) []Event {
-	oldPath, currentPath := w.parsePath(event.Path)
+	oldPath, currentPath := w.parsePath(event.OldPath), w.parsePath(event.Path)
 	if isEventType(event.Op, watcher.Rename, watcher.Move) {
 		return []Event{{Op: Remove, Path: oldPath}, {Op: Update, Path: currentPath, LastKnownChecksum: w.checksums[currentPath]}}
 	} else if isEventType(event.Op, watcher.Remove) {
@@ -165,19 +179,12 @@ func (w *Watcher) translateEvent(event watcher.Event) []Event {
 	return []Event{}
 }
 
-func (w *Watcher) parsePath(path string) (old, current string) {
-	parts := strings.Split(path, filepathSplit)
-	for i, path := range parts {
-		projectPath := pathToProject(w.directory, path)
-		if projectPath == "" {
-			projectPath = path
-		}
-		parts[i] = projectPath
-	}
-	if len(parts) > 1 {
-		return parts[0], parts[1]
+func (w *Watcher) parsePath(path string) string {
+	projectPath := pathToProject(w.directory, path)
+	if projectPath == "" {
+		return path
 	}
-	return "", parts[0]
+	return projectPath
 }
 
 func isEventType(currentOp watcher.Op, allowedOps ...watcher.Op) bool {


Test patch:
diff --git a/src/file/watcher_test.go b/src/file/watcher_test.go
index 84870c8..423517b 100644
--- a/src/file/watcher_test.go
+++ b/src/file/watcher_test.go
@@ -80,7 +80,9 @@ func TestFileWatcher_filterHook(t *testing.T) {
 		{filename: "_testdata/project/config/settings.json", skip: true},
 		{filename: "_testdata/project/config.yml", skip: false},
 		{filename: "_testdata/project/templates/template.liquid", skip: false},
-		{filename: "_testdata/project/templates", skip: true},
+		{filename: "_testdata/project/templates", skip: false},
+		{filename: "_testdata", skip: true},
+		{filename: "_testdata/project/javascripts/app.js", skip: true},
 	}
 
 	hook := createTestFilterHook(t)
@@ -105,7 +107,6 @@ func TestFileWatcher_WatchFsEvents(t *testing.T) {
 		{expectedOp: Update, filename: "_testdata/project/templates/template.liquid", op: watcher.Write},
 		{expectedOp: Update, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Write},
 		{expectedOp: Remove, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Remove},
-		{expectedOp: Remove, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Rename},
 	}
 
 	for i, testcase := range testcases {
@@ -130,23 +131,23 @@ func (p OptSlice) Swap(i, j int)      { p[i], p[j] = p[j], p[i] }
 
 func TestFileWatcher_OnEvent(t *testing.T) {
 	testcases := []struct {
-		filename   string
-		op         watcher.Op
-		expectedOp OptSlice
+		filename, oldname string
+		op                watcher.Op
+		expectedOp        OptSlice
 	}{
 		{expectedOp: OptSlice{Update}, filename: "_testdata/project/templates/template.liquid", op: watcher.Write},
 		{expectedOp: OptSlice{Update}, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Create},
+		{expectedOp: OptSlice{}, filename: "_testdata/project/config", op: watcher.Create},
 		{expectedOp: OptSlice{Remove}, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Remove},
-		{expectedOp: OptSlice{Remove, Update}, filename: "_testdata/project/assets/application.js.liquid -> _testdata/project/assets/application.js", op: watcher.Rename},
-		{expectedOp: OptSlice{Remove, Update}, filename: "_testdata/project/assets/application.js.liquid -> _testdata/project/assets/application.js", op: watcher.Move},
+		{expectedOp: OptSlice{Remove, Update}, filename: "_testdata/project/assets/application.js.liquid", oldname: "_testdata/project/assets/application.js", op: watcher.Rename},
+		{expectedOp: OptSlice{Remove, Update}, filename: "_testdata/project/assets/application.js.liquid", oldname: "_testdata/project/assets/application.js", op: watcher.Move},
 	}
 
 	for i, testcase := range testcases {
 		w := createTestWatcher(t)
 		w.Events = make(chan Event, len(testcases))
-		_, currentPath := w.parsePath(testcase.filename)
-		info, _ := os.Stat(filepath.Join("_testdata", "project", currentPath))
-		w.onEvent(watcher.Event{Op: testcase.op, Path: testcase.filename, FileInfo: info})
+		info, _ := os.Stat(testcase.filename)
+		w.onEvent(watcher.Event{Op: testcase.op, Path: testcase.filename, OldPath: testcase.oldname, FileInfo: info})
 		assert.Equal(t, len(testcase.expectedOp), len(w.Events), fmt.Sprintf("testcase: %v", i))
 
 		recievedEvents := OptSlice{}
@@ -183,17 +184,14 @@ func TestFileWatcher_debouncing(t *testing.T) {
 
 func TestFileWatcher_ParsePath(t *testing.T) {
 	testcases := []struct {
-		input, oldpath, currentpath string
+		input, currentpath string
 	}{
-		{"_testdata/project/assets/app.js", "", "assets/app.js"},
-		{"_testdata/project/assets/app.js -> _testdata/project/assets/app.js.liquid", "assets/app.js", "assets/app.js.liquid"},
-		{"not/another/path/assets/app.js", "", "not/another/path/assets/app.js"},
+		{"_testdata/project/assets/app.js", "assets/app.js"},
+		{"not/another/path/assets/app.js", "not/another/path/assets/app.js"},
 	}
 	w := createTestWatcher(t)
 	for _, testcase := range testcases {
-		o, c := w.parsePath(testcase.input)
-		assert.Equal(t, o, testcase.oldpath)
-		assert.Equal(t, c, testcase.currentpath)
+		assert.Equal(t, w.parsePath(testcase.input), testcase.currentpath)
 	}
 }