From e9ef7b70b82288c731bf1d86402c2395a1d41caf Mon Sep 17 00:00:00 2001 From: xzyfer Date: Fri, 16 Jun 2017 00:50:11 +1000 Subject: [PATCH] Decouple the graph and render logic from the fs watcher This logic is all tightly coupled in the bin. This logic has been notoriously impossible to test due to weird fs timing issues. By extracting the actual logic we're now able to test it in isolation. With this in place replacing Gaze (#636) becomes a viable option. This PR not only massively increases our test coverage for the watcher but also address a bunch of known edge cases i.e. orphaned imports when a files is deleted. Closes #1896 Fixes #1891 --- bin/node-sass | 70 +++---- lib/watcher.js | 84 ++++++++ package.json | 4 +- test/fixtures/watcher/main/one.scss | 5 + test/fixtures/watcher/main/partials/_one.scss | 3 + .../watcher/main/partials/_three.scss | 3 + test/fixtures/watcher/main/partials/_two.scss | 3 + test/fixtures/watcher/main/two.scss | 5 + .../watcher/sibling/partials/_three.scss | 3 + test/fixtures/watcher/sibling/three.scss | 5 + test/watcher.js | 190 ++++++++++++++++++ 11 files changed, 328 insertions(+), 47 deletions(-) create mode 100644 lib/watcher.js create mode 100644 test/fixtures/watcher/main/one.scss create mode 100644 test/fixtures/watcher/main/partials/_one.scss create mode 100644 test/fixtures/watcher/main/partials/_three.scss create mode 100644 test/fixtures/watcher/main/partials/_two.scss create mode 100644 test/fixtures/watcher/main/two.scss create mode 100644 test/fixtures/watcher/sibling/partials/_three.scss create mode 100644 test/fixtures/watcher/sibling/three.scss create mode 100644 test/watcher.js diff --git a/bin/node-sass b/bin/node-sass index e94c12c77..4139f71d1 100755 --- a/bin/node-sass +++ b/bin/node-sass @@ -3,13 +3,13 @@ var Emitter = require('events').EventEmitter, forEach = require('async-foreach').forEach, Gaze = require('gaze'), - grapher = require('sass-graph'), meow = require('meow'), util = require('util'), path = require('path'), glob = require('glob'), sass = require('../lib'), render = require('../lib/render'), + watcher = require('../lib/watcher'), stdout = require('stdout-stream'), stdin = require('get-stdin'), fs = require('fs'); @@ -229,63 +229,41 @@ function getOptions(args, options) { */ function watch(options, emitter) { - var buildGraph = function(options) { - var graph; - var graphOptions = { - loadPaths: options.includePath, - extensions: ['scss', 'sass', 'css'] - }; - - if (options.directory) { - graph = grapher.parseDir(options.directory, graphOptions); - } else { - graph = grapher.parseFile(options.src, graphOptions); - } + var handler = function(files) { + files.added.forEach(function(file) { + var watch = gaze.watched(); + if (watch.indexOf(file) === -1) { + gaze.add(file); + } + }); - return graph; + files.changed.forEach(function(file) { + if (path.basename(file)[0] !== '_') { + renderFile(file, options, emitter); + } + }); + + files.removed.forEach(function(file) { + gaze.remove(file); + }); }; - var watch = []; - var graph = buildGraph(options); - - // Add all files to watch list - for (var i in graph.index) { - watch.push(i); - } + watcher.reset(options); var gaze = new Gaze(); - gaze.add(watch); + gaze.add(watcher.reset(options)); gaze.on('error', emitter.emit.bind(emitter, 'error')); gaze.on('changed', function(file) { - var files = [file]; - - // descendents may be added, so we need a new graph - graph = buildGraph(options); - graph.visitAncestors(file, function(parent) { - files.push(parent); - }); - - // Add children to watcher - graph.visitDescendents(file, function(child) { - if (watch.indexOf(child) === -1) { - watch.push(child); - gaze.add(child); - } - }); - files.forEach(function(file) { - if (path.basename(file)[0] !== '_') { - renderFile(file, options, emitter); - } - }); + handler(watcher.changed(file)); }); - gaze.on('added', function() { - graph = buildGraph(options); + gaze.on('added', function(file) { + handler(watcher.added(file)); }); - gaze.on('deleted', function() { - graph = buildGraph(options); + gaze.on('deleted', function(file) { + handler(watcher.deleted(file)); }); } diff --git a/lib/watcher.js b/lib/watcher.js new file mode 100644 index 000000000..c43552190 --- /dev/null +++ b/lib/watcher.js @@ -0,0 +1,84 @@ +var grapher = require('sass-graph'), + clonedeep = require('lodash.clonedeep'), + watcher = { + opts: {} + }, + graph = null; + +watcher.reset = function(opts) { + this.opts = clonedeep(opts || this.opts || {}); + var options = { + loadPaths: this.opts.includePath, + extensions: ['scss', 'sass', 'css'] + }; + + if (this.opts.directory) { + graph = grapher.parseDir(this.opts.directory, options); + } else { + graph = grapher.parseFile(this.opts.src, options); + } + + return Object.keys(graph.index); +}; + +watcher.changed = function(absolutePath) { + var files = { + added: [], + changed: [], + removed: [], + }; + + this.reset(); + + graph.visitAncestors(absolutePath, function(parent) { + files.changed.push(parent); + }); + + graph.visitDescendents(absolutePath, function(child) { + files.added.push(child); + }); + + return files; +}; + +watcher.added = function(absolutePath) { + var files = { + added: [], + changed: [], + removed: [], + }; + + this.reset(); + + if (Object.keys(graph.index).indexOf(absolutePath) !== -1) { + files.added.push(absolutePath); + } + + graph.visitDescendents(absolutePath, function(child) { + files.added.push(child); + }); + + return files; +}; + +watcher.removed = function(absolutePath) { + var files = { + added: [], + changed: [], + removed: [], + }; + + graph.visitAncestors(absolutePath, function(parent) { + files.changed.push(parent); + }); + + if (Object.keys(graph.index).indexOf(absolutePath) !== -1) { + files.removed.push(absolutePath); + } + + this.reset(); + + return files; +}; + +module.exports = watcher; diff --git a/package.json b/package.json index 7115a33d6..f5c844acc 100644 --- a/package.json +++ b/package.json @@ -75,12 +75,14 @@ "devDependencies": { "coveralls": "^2.11.8", "eslint": "^3.4.0", + "fs-extra": "^0.30.0", "istanbul": "^0.4.2", "mocha": "^3.1.2", "mocha-lcov-reporter": "^1.2.0", "object-merge": "^2.5.1", "read-yaml": "^1.0.0", "rimraf": "^2.5.2", - "sass-spec": "3.5.0-1" + "sass-spec": "3.5.0-1", + "unique-temp-dir": "^1.0.0" } } diff --git a/test/fixtures/watcher/main/one.scss b/test/fixtures/watcher/main/one.scss new file mode 100644 index 000000000..414af5e3f --- /dev/null +++ b/test/fixtures/watcher/main/one.scss @@ -0,0 +1,5 @@ +@import "partials/one"; + +.one { + color: red; +} diff --git a/test/fixtures/watcher/main/partials/_one.scss b/test/fixtures/watcher/main/partials/_one.scss new file mode 100644 index 000000000..b2a72d637 --- /dev/null +++ b/test/fixtures/watcher/main/partials/_one.scss @@ -0,0 +1,3 @@ +.one { + color: darkred; +} diff --git a/test/fixtures/watcher/main/partials/_three.scss b/test/fixtures/watcher/main/partials/_three.scss new file mode 100644 index 000000000..1846e9ae2 --- /dev/null +++ b/test/fixtures/watcher/main/partials/_three.scss @@ -0,0 +1,3 @@ +.three { + color: darkgreen; +} diff --git a/test/fixtures/watcher/main/partials/_two.scss b/test/fixtures/watcher/main/partials/_two.scss new file mode 100644 index 000000000..6a3b4ed90 --- /dev/null +++ b/test/fixtures/watcher/main/partials/_two.scss @@ -0,0 +1,3 @@ +.two { + color: darkblue; +} diff --git a/test/fixtures/watcher/main/two.scss b/test/fixtures/watcher/main/two.scss new file mode 100644 index 000000000..3e9c0fbf9 --- /dev/null +++ b/test/fixtures/watcher/main/two.scss @@ -0,0 +1,5 @@ +@import "partials/two"; + +.two { + color: blue; +} diff --git a/test/fixtures/watcher/sibling/partials/_three.scss b/test/fixtures/watcher/sibling/partials/_three.scss new file mode 100644 index 000000000..1846e9ae2 --- /dev/null +++ b/test/fixtures/watcher/sibling/partials/_three.scss @@ -0,0 +1,3 @@ +.three { + color: darkgreen; +} diff --git a/test/fixtures/watcher/sibling/three.scss b/test/fixtures/watcher/sibling/three.scss new file mode 100644 index 000000000..4e9d1a7e4 --- /dev/null +++ b/test/fixtures/watcher/sibling/three.scss @@ -0,0 +1,5 @@ +@import "partials/three"; + +.three { + color: green; +} diff --git a/test/watcher.js b/test/watcher.js new file mode 100644 index 000000000..7c9135e72 --- /dev/null +++ b/test/watcher.js @@ -0,0 +1,190 @@ +var assert = require('assert'), + fs = require('fs-extra'), + path = require('path'), + temp = require('unique-temp-dir'), + watcher = require('../lib/watcher'); + +describe.only('watcher', function() { + var main, sibling; + var origin = path.join(__dirname, 'fixtures', 'watcher'); + + beforeEach(function() { + var fixture = temp(); + fs.ensureDirSync(fixture); + fs.copySync(origin, fixture); + main = fs.realpathSync(path.join(fixture, 'main')); + sibling = fs.realpathSync(path.join(fixture, 'sibling')); + }); + + describe('with directory', function() { + beforeEach(function() { + watcher.reset({ + directory: main, + loadPaths: [main] + }); + }); + + describe('when a file is changed in the graph', function() { + it('should return the files to compile', function() { + var files = watcher.changed(path.join(main, 'partials', '_one.scss')); + assert.deepEqual(files, { + added: [], + changed: [path.join(main, 'one.scss')], + removed: [], + }); + }); + + describe('and @imports previously not imported files', function() { + it('should also return the new imported files', function() { + var file = path.join(main, 'partials', '_one.scss'); + fs.writeFileSync(file, '@import "partials/three.scss";', { flag: 'a' }); + var files = watcher.changed(file); + assert.deepEqual(files, { + added: [path.join(main, 'partials', '_three.scss')], + changed: [path.join(main, 'one.scss')], + removed: [], + }); + }); + }); + }); + + describe('when a file is changed outside the graph', function() { + it('should return an empty array', function() { + var files = watcher.changed(path.join(sibling, 'partials', '_three.scss')); + assert.deepEqual(files, { + added: [], + changed: [], + removed: [], + }); + }); + }); + + describe('when a file is added in the graph', function() { + it('should return only the added file', function() { + var file = path.join(main, 'three.scss'); + fs.writeFileSync(file, '@import "paritals/two.scss";'); + var files = watcher.added(file); + assert.deepEqual(files, { + added: [file], + changed: [], + removed: [], + }); + }); + + describe('and @imports previously not imported files', function() { + it('should also return the new imported files', function() { + var file = path.join(main, 'three.scss'); + fs.writeFileSync(file, '@import "partials/three.scss";'); + var files = watcher.added(file); + assert.deepEqual(files, { + added: [ + file, + path.join(main, 'partials', '_three.scss') + ], + changed: [], + removed: [], + }); + }); + }); + }); + + describe('when a file is added outside the graph', function() { + it('should return an empty array', function() { + var files = watcher.added(path.join(sibling, 'three.scss')); + assert.deepEqual(files, { + added: [], + changed: [], + removed: [], + }); + }); + }); + + describe('when a file is removed from the graph', function() { + it('should return the files to compile', function() { + var files = watcher.removed(path.join(main, 'partials', '_one.scss')); + assert.deepEqual(files, { + added: [], + changed: [path.join(main, 'one.scss')], + removed: [path.join(main, 'partials', '_one.scss')], + }); + }); + }); + + describe('when a file is removed from outside the graph', function() { + it('should return an empty array', function() { + var files = watcher.removed(path.join(sibling, 'partials', '_three.scss')); + assert.deepEqual(files, { + added: [], + changed: [], + removed: [], + }); + }); + }); + }); + + describe('with file', function() { + beforeEach(function() { + watcher.reset({ + src: path.join(main, 'one.scss'), + loadPaths: [main] + }); + }); + + describe('when a file is changed in the graph', function() { + it('should return the files to compile', function() { + var files = watcher.changed(path.join(main, 'partials', '_one.scss')); + assert.deepEqual(files, { + added: [], + changed: [path.join(main, 'one.scss')], + removed: [], + }); + }); + }); + + describe('when a file is changed outside the graph', function() { + it('should return an empty array', function() { + var files = watcher.changed(path.join(sibling, 'partials', '_three.scss')); + assert.deepEqual(files, { + added: [], + changed: [], + removed: [], + }); + }); + }); + + describe('when a file is added', function() { + it('should return an empty array', function() { + var file = path.join(main, 'three.scss'); + fs.writeFileSync(file, '@import "paritals/one.scss";'); + var files = watcher.added(file); + assert.deepEqual(files, { + added: [], + changed: [], + removed: [], + }); + }); + }); + + describe('when a file is removed from the graph', function() { + it('should return the files to compile', function() { + var files = watcher.removed(path.join(main, 'partials', '_one.scss')); + assert.deepEqual(files, { + added: [], + changed: [path.join(main, 'one.scss')], + removed: [path.join(main, 'partials', '_one.scss')], + }); + }); + }); + + describe('when a file is removed from outside the graph', function() { + it('should return an empty array', function() { + var files = watcher.removed(path.join(sibling, 'partials', '_three.scss')); + assert.deepEqual(files, { + added: [], + changed: [], + removed: [], + }); + }); + }); + }); +});