Fixed watch behavior #782

wesleytodd · 2015-03-22T15:12:31Z

This fixes #781. Using the watch command now finds all of the dependencies and will recompile when one of them changes.

am11 · 2015-03-22T15:40:33Z

test/cli.js

+
+      bin.stdout.setEncoding('utf8');
+      bin.stdout.once('data', function(data) {
+        assert(data.trim() === 'body{background:white}');


After this line, fs.unlinkSync(fixtures('watching/index.css')) and then git rm test/fixtures/watching/index.css to prevent it from bleeding into source-control.

Thanks! Can you please rebase into one commit?

am11 · 2015-03-22T15:59:11Z

Please elaborate, what will happen when:

watch = file.scss and args[0] = file2.scss
watch = directory/ and args[0] = file2.scss

wesleytodd · 2015-03-22T16:18:39Z

watch = file.scss and args[0] = file2.scss

This use case is probably the only way you can go wrong with this. This will recompile file2.scss whenever file.scss changes. I cannot imagine anyone wanting that behavior, but that is the way it works now.

watch = directory/ and args[0] = file2.scss

Similarly to the above, this will recompile file2.scss whenever anything in directory/*.{scss,sass} changes.

wesleytodd · 2015-03-22T16:19:52Z

To be honest this conversation makes we want to change the behavior completely. Seems like --watch should only be a boolean. And arg[0] should only be the files to watch. If you want me to make this PR do that instead I can.

am11 · 2015-03-22T16:45:52Z

Seems like --watch should only be a Boolean

👍 💯

Please go ahead.

As a side note, in case the user has used custom importer option with watch, it will require() that function just once. But while the file is being watched, any change to the importer function would be ignored. To fix this, I think we should unload and re-require the import file before the watcher requests compile-on-change.

Thoughts?

kevva · 2015-03-22T17:43:12Z

bin/node-sass

+  var watchDir = isSassFile(options.watch) ? path.dirname(path.resolve(options.watch)) : options.watch,
+      glob = options.recursive ? '**/*.{sass,scss}' : '*.{sass,scss}',
+      gaze = new Gaze(),
+      dir;


Use multiline var statements.

I had that, but the top of the file was using commas, so I changed it. Are there contribution docs on this stuff that I can read?

wesleytodd · 2015-03-22T19:51:18Z

@am11 I will do that then. I am not sure what the custom importers do, but I am sure I can figure it out, or if you have docs you can point me to. Otherwise, if I have any questions I will be sure to ask them here.

am11 · 2015-03-22T20:25:47Z

@wesleytodd, you can find the usage in example: https://github.com/sass/node-sass/#examples.
When a user of node-sass set the custom importer, LibSass calls the function as it encounters any Sass @import (as opposed to CSS3 import directive). This way user can alter the file-path and read the data (in case the @import had URL -- because LibSass doesn't download files from URLs -- or even database connection string etc.) and return the altered result to LibSass to continue compilation.

Separately, in this PR, you may be able to address #601 by simply retiring --recursive option and make the default behavior recursive. (-8

am11 · 2015-03-28T15:20:10Z

@wesleytodd, any chance we can wrap this up before v3 stable?

wesleytodd · 2015-03-28T16:39:40Z

Sorry, been really busy this week. When is the v3 stable deadline?

am11 · 2015-03-28T17:01:14Z

TYT! 😎 👍

Our stable release (v3.0.0) will be aligned with that of libsass v3.2.0. Going by their milestone, https://github.com/sass/libsass/milestones/3.2, we have probably few more days.

I am sorry, but we have included some new CLI features. You would need to rebase. I hope there won't be many merging issues in your feature branch.

Let us know if you need assistance in moving things forward.

wesleytodd · 2015-03-28T22:31:09Z

Was the cli change anything that effects this feature? The merge was clean but I haven't tested the feature yet.

I will work on getting the new behavior working tomorrow, but if there is anything else I should know about cli changes it would be good to know now :), as I have not been keeping track of the rest of this projects active issues.

wesleytodd · 2015-03-28T23:18:37Z

Nevermind, rebuilding libsass did the trick. I am not used to working on node projects with c deps.

wesleytodd · 2015-03-28T23:52:05Z

Whelp.....I made the mistake of getting io setup on my new laptop, and then found this issue: shama/gaze#173

I asked for their recommendation, so if we cannot get node@0.12.x and io.js support we might want to consider going with something similar to this: latentflip/stylizer#6

Thoughts?

wesleytodd · 2015-03-29T00:26:23Z

So I am no longer sure about that gaze issue. I got it working, I just had a few silly errors that I hadn't noticed in my code. If these tests pass in CI then I think this is ready.

am11 · 2015-03-29T03:48:14Z

Both CI builds are passing. 👍 🎉

Regarding gaze, I saw that issue, but it never failed functioning with io.js et al. on any platform runtime I have used so far. I think it is because the version we are using, v0.5 wasn't relying on C++ module? It is unclear from the history of repo, but going by the code, it seems like it was pure JS module back then. Hence compatibility is not the issue.

Regarding --recursive option, do you think we can live without it, by making recursive a default behavior (and call it a fix for #601)?

wesleytodd · 2015-03-29T04:05:23Z

I think that it makes sense to keep recursive as an option. There is certainly a situation where you might not want to compile files deeper in the tree. But I do think the recursive option is more common and should be default.

am11 · 2015-03-29T04:15:03Z

I agree with you. So can we can probably just set it here: (https://github.com/wesleytodd/node-sass/blob/bd710c61c76d49cd4decdb59b6a8c68bf07b91b6/bin/node-sass#L86) like this:

..
  precision: 5,
  recursive: true
}

Would you like to fix it in this PR? :)

wesleytodd · 2015-03-29T04:23:17Z

Updated.

am11 · 2015-03-29T05:20:27Z

Thanks for your contribution @wesleytodd. 🎉

Looks good to me. 👍

wesleytodd · 2015-03-29T05:57:36Z

@am11 Glad I could help!! Its a good project, so I am sure I will be using it a lot. Maybe find some more time to help out in the future.

am11 · 2015-03-29T07:27:13Z

Maybe find some more time to help out in the future.

Most welcome and keep coming back! 😎 👍

Fixed watch behavior

am11 · 2015-03-29T08:22:57Z

Few things I noticed:

With meow package, the CLI arguments parser, converts all non-true values to false. Meaning:
- node-sass blah.scss --watch --recursive false == node-sass blah.scss --watch --recursive untrue == node-sass blah.scss --watch --recursive foo
- while node-sass blah.scss --watch --recursive true == node-sass blah.scss --watch --recursive == node-sass blah.scss --watch
You are the 100th contributor to this project, so here you go: 💯 🏆 😃

wesleytodd · 2015-03-29T13:07:40Z

@am11 I believe that is the behavior from minimist. This is a very commonly used package, so my guess is this behavior is fine, despite it's oddities. We could submit an issue over there if you have a better suggestion for these behaviors.

Yeah me! Thanks for the 🏆!

wesleytodd · 2015-03-29T13:10:22Z

Also, while pursuing the minimist source I noticed it supports --no-recursive.

And here is the issue in minimist for reference: https://github.com/substack/minimist/blob/master/index.js#L158

am11 · 2015-03-29T14:55:53Z

I agree that --no-recursive would be better suited here, when recursive is a default behavior.

wesleytodd mentioned this pull request Mar 22, 2015

Compile a single file while watching the whole directory #781

Closed

am11 reviewed Mar 22, 2015
View reviewed changes

kevva reviewed Mar 22, 2015
View reviewed changes

wesleytodd force-pushed the watch-fix branch from a841c0f to 16a3212 Compare March 22, 2015 19:52

am11 added Module - CLI Dev - WIP labels Mar 22, 2015

This was referenced Mar 23, 2015

Provide CLI support for Custom Functions #784

Closed

Support for polling interval with --watch #801

Closed

am11 force-pushed the master branch from c07d8d2 to a0bd310 Compare March 25, 2015 21:46

am11 added the Feature - Implementation label Mar 28, 2015

wesleytodd mentioned this pull request Mar 28, 2015

io.js support shama/gaze#173

Closed

wesleytodd force-pushed the watch-fix branch 2 times, most recently from bd1d246 to bd710c6 Compare March 29, 2015 00:23

Converted watch to a boolean and got it actually working correctly

4ed6de2

wesleytodd force-pushed the watch-fix branch from bd710c6 to 4ed6de2 Compare March 29, 2015 04:22

am11 added up-for-grabs and removed Dev - WIP labels Mar 29, 2015

am11 added a commit that referenced this pull request Mar 29, 2015

Merge pull request #782 from wesleytodd/watch-fix

33b0b60

Fixed watch behavior

am11 merged commit 33b0b60 into sass:master Mar 29, 2015

am11 removed the up-for-grabs label Mar 29, 2015

am11 mentioned this pull request Mar 29, 2015

cmd-line: Recursive option doesn't work as expected #601

Closed

am11 added the Release - Breaking Change label Mar 30, 2015

am11 mentioned this pull request Apr 2, 2015

Support building directory/glob of SASS files from CLI #828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed watch behavior #782

Fixed watch behavior #782

wesleytodd commented Mar 22, 2015

am11 Mar 22, 2015

wesleytodd Mar 22, 2015

am11 Mar 22, 2015

am11 commented Mar 22, 2015

wesleytodd commented Mar 22, 2015

wesleytodd commented Mar 22, 2015

am11 commented Mar 22, 2015

kevva Mar 22, 2015

wesleytodd Mar 22, 2015

wesleytodd commented Mar 22, 2015

am11 commented Mar 22, 2015

am11 commented Mar 28, 2015

wesleytodd commented Mar 28, 2015

am11 commented Mar 28, 2015

wesleytodd commented Mar 28, 2015

wesleytodd commented Mar 28, 2015

wesleytodd commented Mar 28, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

am11 commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

Fixed watch behavior #782

Fixed watch behavior #782

Conversation

wesleytodd commented Mar 22, 2015

am11 Mar 22, 2015

Choose a reason for hiding this comment

wesleytodd Mar 22, 2015

Choose a reason for hiding this comment

am11 Mar 22, 2015

Choose a reason for hiding this comment

am11 commented Mar 22, 2015

wesleytodd commented Mar 22, 2015

wesleytodd commented Mar 22, 2015

am11 commented Mar 22, 2015

kevva Mar 22, 2015

Choose a reason for hiding this comment

wesleytodd Mar 22, 2015

Choose a reason for hiding this comment

wesleytodd commented Mar 22, 2015

am11 commented Mar 22, 2015

am11 commented Mar 28, 2015

wesleytodd commented Mar 28, 2015

am11 commented Mar 28, 2015

wesleytodd commented Mar 28, 2015

wesleytodd commented Mar 28, 2015

wesleytodd commented Mar 28, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015

am11 commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

wesleytodd commented Mar 29, 2015

am11 commented Mar 29, 2015