Skip to content
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

Merged
merged 1 commit into from
Jun 17, 2017

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Jun 15, 2017

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

@xzyfer xzyfer requested a review from nschonni June 15, 2017 14:58
@xzyfer xzyfer force-pushed the watch-me-if-you-can branch 2 times, most recently from 31d7d42 to d3450bc Compare June 15, 2017 15:25
@@ -75,12 +75,14 @@
"devDependencies": {
"coveralls": "^2.11.8",
"eslint": "^3.4.0",
"fs-extra": "^0.30.0",
Copy link
Contributor Author

@xzyfer xzyfer Jun 15, 2017

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.

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
Copy link
Contributor

@nschonni nschonni left a 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:

  1. Abstract all the gaze.xxx calls in bin/node-sass to functions in the watcher
  2. Extract the watcher to a new NPM module

@xzyfer
Copy link
Contributor Author

xzyfer commented Jun 16, 2017

Abstract all the gaze.xxx calls in bin/node-sass to functions in the watcher

Can you elaborate on how this differs from what I've done here?

Extract the watcher to a new NPM module

This is related to #1056. I'm open to it.

}
var handler = function(files) {
files.added.forEach(function(file) {
var watch = gaze.watched();
Copy link
Contributor

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

Copy link
Contributor Author

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.

@nschonni
Copy link
Contributor

nschonni commented Jun 16, 2017

The other part of splitting it would be a conditional require

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] !== '_') {
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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

@nschonni
Copy link
Contributor

I think this is probably fine to land as-is as it gets us testing.
Looking at the bin again, if we do a second pass, I think all of the gaze/watcher may just be pulled out by changing https://github.com/xzyfer/node-sass/blob/e81d6b9f225be4ea124a244d6e287a3711044e9f/bin/node-sass#L312

   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

@xzyfer
Copy link
Contributor Author

xzyfer commented Jun 16, 2017

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.

@xzyfer xzyfer merged commit aadf8d4 into sass:master Jun 17, 2017
@xzyfer xzyfer deleted the watch-me-if-you-can branch June 17, 2017 09:06
Friendly-users referenced this pull request in Friendly-users/node-sass Jul 9, 2024
-----
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants