Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Compile a single file while watching the whole directory #781

Closed
wesleytodd opened this issue Mar 21, 2015 · 13 comments · Fixed by #782
Closed

Compile a single file while watching the whole directory #781

wesleytodd opened this issue Mar 21, 2015 · 13 comments · Fixed by #782

Comments

@wesleytodd
Copy link
Contributor

I have been digging around in here for a while but didn't an existing issue for this. Here is the command I am running:

$ node-sass --watch=sass --output-style=expanded --source-map="main.css.map" --recursive sass/main.scss -o build

It correctly builds the file and the source map. But it does not watch the other files imported by main.scss. I tracked down the problem to this line:

var graph = grapher.parseDir(options.src, { loadPaths: options.includePath });

I believe this is supposed to be:

var graph = grapher.parseDir(options.watch, { loadPaths: options.includePath });

This fixes my issue, but I don't know if this breaks other use cases. The tests seem to be failing before making the above change, but no new failures happen when I run it with the patch. Specifically the one that tests watched files still passes. Other than that I am not entirely sure which manual cases to test.

Should I submit a PR for this? Is there something I am missing about how this is working? Thanks for any help and or direction you can point me in.

@am11
Copy link
Contributor

am11 commented Mar 21, 2015

This issue was actually reported differently at #755. It was fixed with: 7a9238d and we released the fix in v3.0.0-alpha.0.

Please try with https://github.com/sass/node-sass/releases/tag/v3.0.0-alpha.1.

@wesleytodd
Copy link
Contributor Author

Still not working for me:

$ node-sass --watch="client/sass" --output-style="expanded" --source-map="build/main.css.map" --source-map-embed --source-map-contents --recursive client/sass/main.scss -o build

=> changed: /Users/westodd/MusicMap/Website/client/sass/main.scss
{
  "message": "File to read not found or unreadable: /Users/westodd/MusicMap/Website/client/sass",
  "status": 4
}

But if I also make the following two changes it works just fine:

/bin/node-sass:40

options.src = args[0] || options.watch;

/bin/node-sass:170

var graph = grapher.parseDir(options.watch, { loadPaths: options.includePath });

@am11
Copy link
Contributor

am11 commented Mar 21, 2015

The correct usage is to use either node-sass watch=blah.scss or without watch node-sass blah.scss. If you provide both, then the behavior is not guaranteed (as we cannot determine what is your intention, compiled the watched file or input file).

@wesleytodd
Copy link
Contributor Author

The intended behavior, which is achieved by these changes, is to recompile the single file, blah.scss, when any file in it's dep tree changes.

The first fix makes the sass-graph module correctly assemble the dep tree. Without it it looks for files like blah.scss/**/*.{sass|scss}. Because it thinks the path it got is a directory, not a single file.

@wesleytodd
Copy link
Contributor Author

The change also allows for the format of node-sass watch=blah.scss to watch and compile a single file, but it doesn't actually find the sass deps correctly....so it is kind of broken. It would take a PR to that module to fully fix this.

@am11
Copy link
Contributor

am11 commented Mar 21, 2015

That makes sense! :)

CLI is a mess at this point, this is also related: #601. As you can see, I attempted to actually support multi-files compilation with or without watch using globs (minimatch), but hit stoppers because of the complexity to determine the destination location.

I think we can have glob pattern support for single file compilation with multi-directory watch.

PRs are most welcome! 👍

@wesleytodd
Copy link
Contributor Author

Yeah, this whole feature is pretty complicated. I honestly think that the main Sass implementation is broken to, or at least overly confusing.

I think that this change might fix most of these features. I have a branch that I will set up a few reduced test cases for and see if they work. Expect a PR soon!!

@am11
Copy link
Contributor

am11 commented Mar 21, 2015

Sure, no hurry. At this point there is another issue; the master is getting segmentation faults with latest libsass changes. So the tests are interrupted. We are waiting for #644, once that get merged, we will start merging other PRs and add new libsass API features.
Meanwhile, you can separate the tests and confirm if they are passing locally because they might not get chance on CI (TravisCI chokes before CLI suite get a chance).

@am11
Copy link
Contributor

am11 commented Mar 21, 2015

We can actually take inspiration from Less' CLI: http://lesscss.org/usage/#command-line-usage, since it is one the closest kind of package we have in npm universe, and I am personally in favor of reconciling Less and Sass usage. (disclaimer: some people might have strong opinions about Less vs. Sass, but they are different in a way that Less is declarative while Sass is an imperative language, both possess advantages over each other and both are admirable..)

The other related model packages (which offer trans-compilations) are: CoffeeScript, LiveScript, SweetJS and TypeScript. For custom importers and custom function, I took ideas from node-jscs, jshint and tslint (and they will also come handy when we implement the lint mode: #680).

@wesleytodd
Copy link
Contributor Author

Yeah, I am getting the seg faults. But the test for this feature are running just fine.

Now that I am digging in and writing tests for this, it turns out that if we get the main way you mentioned working correctly it fixes my use case as well. Anyway, submitting the PR now.

I looked through the less docs, I have never used less, go team Sass ;), but I didn't see a watch command. Maybe I just missed it.

To be honest, now that I got this working I think it makes sense to be able to just do node-sass --watch=main.css, and as long as the watch gets the right deps I am happy. The whole globbing patterns thing is nice, but for the effort I am willing to leave it at this. Maybe someday I can take another look to get that working.

@wesleytodd
Copy link
Contributor Author

Should I just PR agains master?

@am11
Copy link
Contributor

am11 commented Mar 22, 2015

Yes master is the target merge branch.

@wesleytodd
Copy link
Contributor Author

Fixed in #782

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants