Skip to content

Commit e81d6b9

Browse files
committed
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 (sass#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 sass#1896 Fixes sass#1891
1 parent ae4f935 commit e81d6b9

File tree

11 files changed

+327
-47
lines changed

11 files changed

+327
-47
lines changed

Diff for: bin/node-sass

+24-46
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@
33
var Emitter = require('events').EventEmitter,
44
forEach = require('async-foreach').forEach,
55
Gaze = require('gaze'),
6-
grapher = require('sass-graph'),
76
meow = require('meow'),
87
util = require('util'),
98
path = require('path'),
109
glob = require('glob'),
1110
sass = require('../lib'),
1211
render = require('../lib/render'),
12+
watcher = require('../lib/watcher'),
1313
stdout = require('stdout-stream'),
1414
stdin = require('get-stdin'),
1515
fs = require('fs');
@@ -229,63 +229,41 @@ function getOptions(args, options) {
229229
*/
230230

231231
function watch(options, emitter) {
232-
var buildGraph = function(options) {
233-
var graph;
234-
var graphOptions = {
235-
loadPaths: options.includePath,
236-
extensions: ['scss', 'sass', 'css']
237-
};
238-
239-
if (options.directory) {
240-
graph = grapher.parseDir(options.directory, graphOptions);
241-
} else {
242-
graph = grapher.parseFile(options.src, graphOptions);
243-
}
232+
var handler = function(files) {
233+
files.added.forEach(function(file) {
234+
var watch = gaze.watched();
235+
if (watch.indexOf(file) === -1) {
236+
gaze.add(file);
237+
}
238+
});
244239

245-
return graph;
240+
files.changed.forEach(function(file) {
241+
if (path.basename(file)[0] !== '_') {
242+
renderFile(file, options, emitter);
243+
}
244+
});
245+
246+
files.removed.forEach(function(file) {
247+
gaze.remove(file);
248+
});
246249
};
247250

248-
var watch = [];
249-
var graph = buildGraph(options);
250-
251-
// Add all files to watch list
252-
for (var i in graph.index) {
253-
watch.push(i);
254-
}
251+
watcher.reset(options);
255252

256253
var gaze = new Gaze();
257-
gaze.add(watch);
254+
gaze.add(watcher.reset(options));
258255
gaze.on('error', emitter.emit.bind(emitter, 'error'));
259256

260257
gaze.on('changed', function(file) {
261-
var files = [file];
262-
263-
// descendents may be added, so we need a new graph
264-
graph = buildGraph(options);
265-
graph.visitAncestors(file, function(parent) {
266-
files.push(parent);
267-
});
268-
269-
// Add children to watcher
270-
graph.visitDescendents(file, function(child) {
271-
if (watch.indexOf(child) === -1) {
272-
watch.push(child);
273-
gaze.add(child);
274-
}
275-
});
276-
files.forEach(function(file) {
277-
if (path.basename(file)[0] !== '_') {
278-
renderFile(file, options, emitter);
279-
}
280-
});
258+
handler(watcher.changed(file));
281259
});
282260

283-
gaze.on('added', function() {
284-
graph = buildGraph(options);
261+
gaze.on('added', function(file) {
262+
handler(watcher.added(file));
285263
});
286264

287-
gaze.on('deleted', function() {
288-
graph = buildGraph(options);
265+
gaze.on('deleted', function(file) {
266+
handler(watcher.deleted(file));
289267
});
290268
}
291269

Diff for: lib/watcher.js

+83
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
var grapher = require('sass-graph'),
2+
clonedeep = require('lodash.clonedeep'),
3+
config = {},
4+
watcher = {},
5+
graph = null;
6+
7+
watcher.reset = function(opts) {
8+
config = clonedeep(opts || config || {});
9+
var options = {
10+
loadPaths: config.includePath,
11+
extensions: ['scss', 'sass', 'css']
12+
};
13+
14+
if (config.directory) {
15+
graph = grapher.parseDir(config.directory, options);
16+
} else {
17+
graph = grapher.parseFile(config.src, options);
18+
}
19+
20+
return Object.keys(graph.index);
21+
};
22+
23+
watcher.changed = function(absolutePath) {
24+
var files = {
25+
added: [],
26+
changed: [],
27+
removed: [],
28+
};
29+
30+
this.reset();
31+
32+
graph.visitAncestors(absolutePath, function(parent) {
33+
files.changed.push(parent);
34+
});
35+
36+
graph.visitDescendents(absolutePath, function(child) {
37+
files.added.push(child);
38+
});
39+
40+
return files;
41+
};
42+
43+
watcher.added = function(absolutePath) {
44+
var files = {
45+
added: [],
46+
changed: [],
47+
removed: [],
48+
};
49+
50+
this.reset();
51+
52+
if (Object.keys(graph.index).indexOf(absolutePath) !== -1) {
53+
files.added.push(absolutePath);
54+
}
55+
56+
graph.visitDescendents(absolutePath, function(child) {
57+
files.added.push(child);
58+
});
59+
60+
return files;
61+
};
62+
63+
watcher.removed = function(absolutePath) {
64+
var files = {
65+
added: [],
66+
changed: [],
67+
removed: [],
68+
};
69+
70+
graph.visitAncestors(absolutePath, function(parent) {
71+
files.changed.push(parent);
72+
});
73+
74+
if (Object.keys(graph.index).indexOf(absolutePath) !== -1) {
75+
files.removed.push(absolutePath);
76+
}
77+
78+
this.reset();
79+
80+
return files;
81+
};
82+
83+
module.exports = watcher;

Diff for: package.json

+3-1
Original file line numberDiff line numberDiff line change
@@ -75,12 +75,14 @@
7575
"devDependencies": {
7676
"coveralls": "^2.11.8",
7777
"eslint": "^3.4.0",
78+
"fs-extra": "^0.30.0",
7879
"istanbul": "^0.4.2",
7980
"mocha": "^3.1.2",
8081
"mocha-lcov-reporter": "^1.2.0",
8182
"object-merge": "^2.5.1",
8283
"read-yaml": "^1.0.0",
8384
"rimraf": "^2.5.2",
84-
"sass-spec": "3.5.0-1"
85+
"sass-spec": "3.5.0-1",
86+
"unique-temp-dir": "^1.0.0"
8587
}
8688
}

Diff for: test/fixtures/watcher/main/one.scss

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "partials/one";
2+
3+
.one {
4+
color: red;
5+
}

Diff for: test/fixtures/watcher/main/partials/_one.scss

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
.one {
2+
color: darkred;
3+
}

Diff for: test/fixtures/watcher/main/partials/_three.scss

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
.three {
2+
color: darkgreen;
3+
}

Diff for: test/fixtures/watcher/main/partials/_two.scss

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
.two {
2+
color: darkblue;
3+
}

Diff for: test/fixtures/watcher/main/two.scss

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "partials/two";
2+
3+
.two {
4+
color: blue;
5+
}

Diff for: test/fixtures/watcher/sibling/partials/_three.scss

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
.three {
2+
color: darkgreen;
3+
}

Diff for: test/fixtures/watcher/sibling/three.scss

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
@import "partials/three";
2+
3+
.three {
4+
color: green;
5+
}

0 commit comments

Comments
 (0)