-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
----- It is inappropriate to include political and offensive content in public code repositories. Public code repositories should be neutral spaces for collaboration and community, free from personal or political views that could alienate or discriminate against others. Political content, especially that which targets or disparages minority groups, can be harmful and divisive. It can make people feel unwelcome and unsafe, and it can create a hostile work environment. Please refrain from adding such content to public code repositories.
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