-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Decouple the graph and render logic from the fs watcher #2020
Conversation
31d7d42
to
d3450bc
Compare
@@ -75,12 +75,14 @@ | |||
"devDependencies": { | |||
"coveralls": "^2.11.8", | |||
"eslint": "^3.4.0", | |||
"fs-extra": "^0.30.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a super older version but it's the last version that supports Node 0.10. It's only for tests so I'm not too fussed.
d3450bc
to
e9ef7b7
Compare
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
e9ef7b7
to
e81d6b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. But here is an alternate take/suggestion:
- Abstract all the
gaze.xxx
calls in bin/node-sass to functions in the watcher - Extract the watcher to a new NPM module
Can you elaborate on how this differs from what I've done here?
This is related to #1056. I'm open to it. |
} | ||
var handler = function(files) { | ||
files.added.forEach(function(file) { | ||
var watch = gaze.watched(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstract was probably the wrong word, but here is a crude example example
var watch = watcher.watched()
if (watch.indexOf(file) === -1) {
watcher.add(file);
}
});
So the watcher
module would export certain functions required in the bin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm still not following.
If I'm understanding correctly wouldn't that then couple us to the fs watcher implementation's api?
What happens if the next fs watcher doesn't expose a method like watch()
for exposing all currently watched files?
The watch.indexOf(file) === -1
is an optimisation to prevent calling gaze.add
superfluously. Under the covers gaze.add
also does deduping but there's not guarantee the next fs watcher would.
The other part of splitting it would be a conditional var watcher;
if (option.watcher !== null)
watcher = require(option.watcher)
// error handling
} else {
watcher = require('node-sass-watcher');
} |
|
||
return graph; | ||
files.changed.forEach(function(file) { | ||
if (path.basename(file)[0] !== '_') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could move this logic into lib/watcher
but if we were to extract it into it's own package I can see uses cases for people wanting to poke changed partials i.e. url
rewriting, or some kind webpack magic.
} | ||
}); | ||
|
||
files.removed.forEach(function(file) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be
files.removed.forEach(gaze.remove)
but it's not likely the next watcher would have the same API
I think this is probably fine to land as-is as it gets us testing. if (options.watch) {
watcher.handle(options, emitter);
} else if (options.directory) {
renderDir(options, emitter);
} else {
render(options, emitter);
} Although I may be missing an obvious problem with passing it that way |
Yeah there will be more passes. I also want to extract the console emiiter stuff because it's the cause of a lot of issues and feature request. |
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