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

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

Closed
egtann opened this issue Jan 4, 2015 · 17 comments
Closed

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

egtann opened this issue Jan 4, 2015 · 17 comments

Comments

@egtann
Copy link

egtann commented Jan 4, 2015

Tested with node-sass@1.2.3 and @2.0.0-beta, which both seem to have the issue.

Steps to reproduce:

$ npm install -g node-sass
$ node-sass -r app/styles OR ./app/styles OR app/styles/ OR ./app/styles/
> Error: File to read not found or unreadable: app/styles/

$ node-sass app/styles/*.scss
> Works fine

Expected behavior: I should be able to pass in a folder with the recursive option.

Edit: I see now that --recursive is just for watching files. You might consider making it apply to inputs as well, which is the expected behavior from normal cmd-line tools (cp, chmod, etc.).

@egtann egtann changed the title cmd-line: Recursive command broken cmd-line: Recursive option broken Jan 4, 2015
@egtann egtann changed the title cmd-line: Recursive option broken cmd-line: Recursive option doesn't work as expected Jan 4, 2015
@kevva
Copy link
Member

kevva commented Jan 4, 2015

You might need to use it along with the --watch flag.

@egtann
Copy link
Author

egtann commented Jan 4, 2015

Right, but this is part of a package.json build script, so I'm not watching the files.

Watch and recursive should probably be independent options. If not, perhaps the docs/command should be updated to the following?

-w  Watch
-wr  Watch recursively

@kevva
Copy link
Member

kevva commented Jan 4, 2015

Nah, but --recursive should definitely work without --watch though.

@egtann
Copy link
Author

egtann commented Jan 4, 2015

Also, even with *.scss, it only outputs the first file it finds and saves it as .scss, overwriting the original file. Perhaps this is a different (but serious) issue.

$ ls .
> base.scss       list_item.scss

$ node-sass -r *.scss -o ./
> Rendering Complete, saving .css file...
> Wrote CSS to list_item.scss

Two problems:

  1. Notice that it outputs to .scss, overwriting my original file! Good thing I had it in a Vim buffer...
  2. It only performs the * match on the first file it finds, and then it exits.

Given the seriousness of the bug (it permanently deletes my un-committed changes without warning), I don't feel comfortable trusting this module in my build script. For now I'm switching back to plain .css.

@am11 am11 added this to the v2.1 milestone Jan 24, 2015
@am11
Copy link
Contributor

am11 commented Feb 1, 2015

@egtann, for the original issue reported. I would consider as a feature request (to provide recursively compile all files in given directory structure) rather than a bug. :)

I have added it in 2.1 milestone. https://github.com/sass/node-sass/milestones.

The second issue you have reported is fixed. I don't have the exact commit reference handy But this is what I get with the latest build (v2.0.0-beta):

C:\temp1> .\node_modules\.bin\node-sass --version
2.0.0-beta

C:\temp1> .\node_modules\.bin\node-sass -r \temp\foo.scss -o ./
Rendering Complete, saving .css file...
Wrote CSS to foo.css

Also, we do not have glob paths support as well as directory for input file yet. For watch, we certainly do: node-sass#L170.
This will be a nice feature!

This may also effect our watch implementation (#629) as follow:

  • Do not allow -w without specifying the input path (be it ./).

This way user would be able to watch any directory and output to any directory, regardless of process.cwd().

Thoughts?

//cc @kevva, @xzyfer

@xzyfer
Copy link
Contributor

xzyfer commented Feb 1, 2015

To be honest I'm not sure why there is a --recursive option. What's the use case where you wouldn't want recursive watching?

I gut feeling is that this flag is a hold over from the broken watcher implementation pre 2.0.0. With the new smart watcher my vote would be for removing the recursive option and being recursive by default.

@am11
Copy link
Contributor

am11 commented Feb 1, 2015

@xzyfer, do you think retiring --recursive in favor of --no-recursive would make sense?

I believe there are use cases where the user would want to watch-compile or instant-compile (this feature request) only the current directory.

Going by laws of "separation of concern", I also think having a separate no resursive for watch and instant (regular/no-watch) options would make sense (although they are mutually exclusive use-cases: watch-compile & non-watch-compile).

@xzyfer
Copy link
Contributor

xzyfer commented Feb 1, 2015

I'm unconvinced there's a use case for non recursive watching or
compilation except speed, which is a non issue IMHO.

I consider Ruby Sass as the gold standard here because it covers 5 years of
use cases and user work flows.

I'm not saying if you give people the option they won't use it, but I don't
think anyone needs it.
On 1 Feb 2015 19:50, "Adeel Mujahid" notifications@github.com wrote:

@xzyfer https://github.com/xzyfer, do you think retiring --recursive in
favor of --no-recursive would make sense?

I believe there are use cases where the user would want to watch-compile
or instant-compile (this feature request) only the current directory.

Going by laws of separation of concerns, I also think having a separate no
resursive
for watch and instant (regular/no-watch) options would make
sense.


Reply to this email directly or view it on GitHub
#601 (comment).

@am11
Copy link
Contributor

am11 commented Feb 2, 2015

@xzyfer, that makes sense. I think we can live without non-recursion if Sassy Ruby say so. :)

@am11 am11 added v2.1 and removed v2.1 labels Feb 12, 2015
@paulcpederson
Copy link
Contributor

I also need to build multiple sass files into multiple css files. Is that possible right now?

Something like:

node-sass sass-dir/*.scss -o css/

or even:

node-sass sass-dir/ -o css/

@paulcpederson
Copy link
Contributor

Also, I agree with @xzyfer that non-recursive watching is probably not needed. I've never needed that.

@peterjmag
Copy link
Contributor

The syntax surrounding watching files and directories is confusing and mostly undocumented. Is there any reason not to use the same CLI syntax that Ruby sass uses?

You can also tell Sass to watch the file and update the CSS every time the Sass file changes:
sass --watch input.scss:output.css
If you have a directory with many Sass files, you can also tell Sass to watch the entire directory:
sass --watch app/sass:public/stylesheets

(from http://sass-lang.com/documentation/file.SASS_REFERENCE.html#using_sass)

$ sass --help
Usage: sass [options] [INPUT] [OUTPUT]
[...]
Watching and Updating:
        --watch                      Watch files or directories for changes.
                                     The location of the generated CSS can be set using a colon:
                                       sass --watch input.sass:output.css
                                       sass --watch input-dir:output-dir

I would at least expect to be able to do something like node-sass -w styles/scss/ -r -o styles/css/, but that doesn't get me very far:

$ node-sass -w styles/scss/ -r -o styles/css/
Provide a Sass file to render

  Example
    node-sass --output-style compressed foobar.scss foobar.css
    cat foobar.scss | node-sass --output-style compressed > foobar.css

Either way, we should agree on a clear syntax and document it properly.

@peterjmag
Copy link
Contributor

I also agree with @xzyfer that --recursive should be the default behavior.

@am11
Copy link
Contributor

am11 commented Feb 21, 2015

@egtann said:

node-sass app/styles/*.scss
Works fine

I do not think it does? We do not support multi-file compilation nor do we support glob in CLI arguments. However, we do create glob magic internally and then pass it to gaze to resolve, only when watch option --watch, -w is set.

Nonetheless, I tried implementing multi-file compilation with globs and no matter how we slice it, there are blockers like:

(Considering glob can accept patterns like: {**/styles/*, /totally/different/path/{*.sass, *.scss}, /blah/**})

  • how to know the user's intent where to place the compiled and sourcemap files?
  • how to calculate the destination path without overwriting existing files?

For this, I solved the least common denominator problem for FS paths but still it will not help as inevitably it will present another set of customizations demand, which is way out of scope IMO.

So frankly the original concern to use recursive without watch, conversely: multi-file compilation support, cannot be entertained.

The solution is to use bash/cmd loops or grep to invoke node-sass iteratively (or recursively) for each file falling under the intended pattern.

Does anyone have objection with this?


Other things that can be done to improve current CLI experience:

  • Remove --recursive option and make recursive the default behaviour.
  • Remove --output and detect if the target is a dir, append the source filename by changing the ext: style.scss <=> style.css.
    • Error if --watch is set to dir and destination is not set to file (directory expected).
  • Make stdout a default behaviour when destination is missing.

Thoughts / ideas?

@paulcpederson
Copy link
Contributor

For those watching this thread who need multiple-file compilation, I wrote a small cli that provides that functionality: https://github.com/paulcpederson/scss-cli

Made it for my own use, but published to npm in case others needed this type of functionality. Right now it has no watch capabilities.

@igregson
Copy link

@paulcpederson many thanks for scss-cli

@am11
Copy link
Contributor

am11 commented Mar 29, 2015

This is fixed by 4ed6de2 via #782.

Credit goes to @wesleytodd.

We finally ended up making the recursive a default option and decided not to drop it as some people still want this behavior. Apparently, people are not 100% satisfied with ruby-sass CLI either..

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

No branches or pull requests

7 participants