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

CLI: Adds support for multi-file compilation. #838

Merged
merged 1 commit into from Apr 7, 2015

Conversation

Projects
None yet
3 participants
@paulcpederson
Contributor

paulcpederson commented Apr 6, 2015

As per #828 - Adds the ability to compile multiple files from a directory.

Given this structure:

- sass/
| - main.scss
|  | - components/
|  |  | - _component1.scss
|  | - pages/
|  |  | - page1.scss
|  |  | - page2.scss

If you run: node-sass sass/ -o build/, you would get this output:

- sass/
- build/
| - main.css
|  | - pages/
|  |  | - page1.css
|  |  | - page2.css

Skips files that start with an _. Also respects the current --recursive flag. So given the same structure as above, if you ran node-sass sass/ -o build/ --recursive false, you would get:

- sass/
- build/
| - main.css
  • Throws an error if you don't difine --output
  • Throws an error if --output isn't a directory
  • If --output is defined, but the folder doesn't exist, it will create it.

Very much looking for feedback. Things that feel specifically weird:

  1. The fact that done is declared as a variable in the root scope of the file. (feels hacky)
  2. The fact that this is waiting for each file to be finished before compiling the next one. (could probably be done in parallel?)
Show outdated Hide outdated bin/node-sass
isDir = true;
} else {
isDir = false;
}

This comment has been minimized.

@am11

am11 Apr 6, 2015

Member
catch (e) {
  isDir = e.code === 'ENOENT';
}
@am11

am11 Apr 6, 2015

Member
catch (e) {
  isDir = e.code === 'ENOENT';
}
Show outdated Hide outdated bin/node-sass
done = this.async();
renderFile(file, options, emitter)
},
function(successful, arr) {

This comment has been minimized.

@am11

am11 Apr 6, 2015

Member
  renderFile(file, options, emitter);
}, function(_, arr) {
@am11

am11 Apr 6, 2015

Member
  renderFile(file, options, emitter);
}, function(_, arr) {
Show outdated Hide outdated bin/node-sass
emitter.emit('error', 'No input file was found.');
}
forEach(files, function(file) {
done = this.async();

This comment has been minimized.

@am11

am11 Apr 6, 2015

Member

Just a convention I picked to refer first argument of all predicates as subject.

forEach(files, function(file) {

to:

forEach(files, function(subject) {

:-)

@am11

am11 Apr 6, 2015

Member

Just a convention I picked to refer first argument of all predicates as subject.

forEach(files, function(file) {

to:

forEach(files, function(subject) {

:-)

Show outdated Hide outdated bin/node-sass
emitter.emit('error', 'Specify an output directory when using multi-file compilation');
}
if (!isDirectory(options.output)) {
emitter.emit('error', ['Multi-file compilation: requires destination', options.output, 'to be a directory.'].join(' '));

This comment has been minimized.

@am11

am11 Apr 6, 2015

Member

Should we return error emission? Like so:

return emitter.emit('error..
@am11

am11 Apr 6, 2015

Member

Should we return error emission? Like so:

return emitter.emit('error..

This comment has been minimized.

@wesleytodd

wesleytodd Apr 6, 2015

Contributor

Also, I am not sure what the standard in node-sass is, but I prefer to use util.format for messages like this. It is what node core does: https://github.com/joyent/node/blob/ef4344311e19a4f73c031508252b21712b22fe8a/lib/tls.js#L152

@wesleytodd

wesleytodd Apr 6, 2015

Contributor

Also, I am not sure what the standard in node-sass is, but I prefer to use util.format for messages like this. It is what node core does: https://github.com/joyent/node/blob/ef4344311e19a4f73c031508252b21712b22fe8a/lib/tls.js#L152

This comment has been minimized.

@paulcpederson

paulcpederson Apr 6, 2015

Contributor

Yeah, saw this style elsewhere in the file. Happy to use util though 👍

@paulcpederson

paulcpederson Apr 6, 2015

Contributor

Yeah, saw this style elsewhere in the file. Happy to use util though 👍

This comment has been minimized.

@paulcpederson

paulcpederson Apr 6, 2015

Contributor

Actually, should I just convert all of these to use util?

@paulcpederson

paulcpederson Apr 6, 2015

Contributor

Actually, should I just convert all of these to use util?

This comment has been minimized.

@am11

am11 Apr 6, 2015

Member

👍
We should refactor the code with formatting error. I just followed @kevva's lead on that one 😜
(we have used that array-join thingy a lot -- all over the place!)

@am11

am11 Apr 6, 2015

Member

👍
We should refactor the code with formatting error. I just followed @kevva's lead on that one 😜
(we have used that array-join thingy a lot -- all over the place!)

@am11

This comment has been minimized.

Show comment
Hide comment
@am11

am11 Apr 6, 2015

Member

This is a great contribution to node-sass. Thank you from the whole Sass community! 😎 👍

Regarding parallel compilation, I second that will help the performance. Using async-foreach module, is it not happening already?

@kevva, @wesleytodd, please take a look.

Member

am11 commented Apr 6, 2015

This is a great contribution to node-sass. Thank you from the whole Sass community! 😎 👍

Regarding parallel compilation, I second that will help the performance. Using async-foreach module, is it not happening already?

@kevva, @wesleytodd, please take a look.

@paulcpederson

This comment has been minimized.

Show comment
Hide comment
@paulcpederson

paulcpederson Apr 6, 2015

Contributor

Cool! All great suggestions (especially re: subject of predicates 👍 ), updating pr now.

Contributor

paulcpederson commented Apr 6, 2015

Cool! All great suggestions (especially re: subject of predicates 👍 ), updating pr now.

Show outdated Hide outdated bin/node-sass
@@ -204,6 +246,15 @@ function run(options, emitter) {
options.includePath = [options.includePath];
}
if(isDirectory(options.src)) {

This comment has been minimized.

@wesleytodd

wesleytodd Apr 6, 2015

Contributor

Since below you are just relying on options.directory being set, seems like you could just do that here to.

@wesleytodd

wesleytodd Apr 6, 2015

Contributor

Since below you are just relying on options.directory being set, seems like you could just do that here to.

@paulcpederson

This comment has been minimized.

Show comment
Hide comment
@paulcpederson

paulcpederson Apr 6, 2015

Contributor

Ok, I went ahead and replace some other instances of the .join style string interpolation with the util.format style mentioned by @wesleytodd in other parts of the cli as well.

Contributor

paulcpederson commented Apr 6, 2015

Ok, I went ahead and replace some other instances of the .join style string interpolation with the util.format style mentioned by @wesleytodd in other parts of the cli as well.

Show outdated Hide outdated bin/node-sass
* @api private
*/
function renderDir(options, emitter) {
var globPath = path.resolve(process.cwd(), options.src, globPattern(options));

This comment has been minimized.

@wesleytodd

wesleytodd Apr 6, 2015

Contributor

For clarity, shouldn't options.src here use options.directory instead? I know they are equal, but it might read more clearly for the next person who comes along.

@wesleytodd

wesleytodd Apr 6, 2015

Contributor

For clarity, shouldn't options.src here use options.directory instead? I know they are equal, but it might read more clearly for the next person who comes along.

This comment has been minimized.

@paulcpederson

paulcpederson Apr 6, 2015

Contributor

great idea. that would be rather confusing.

@paulcpederson

paulcpederson Apr 6, 2015

Contributor

great idea. that would be rather confusing.

@wesleytodd

This comment has been minimized.

Show comment
Hide comment
@wesleytodd

wesleytodd Apr 6, 2015

Contributor

@paulcpederson I see what you mean about the done function, seems like this would be a good situation to use EventEmitter.once. You could remove the disconnected calling of done by putting it right next to the related code:

  forEach(files, function(file) {
      emitter.once('done', this.async());
      renderFile(file, options, emitter);  // Note you were missing a semicolon here :)
    },

I think you will still need the !options.directory line in the other event handler, but it gets rid of the need for the "global".

Contributor

wesleytodd commented Apr 6, 2015

@paulcpederson I see what you mean about the done function, seems like this would be a good situation to use EventEmitter.once. You could remove the disconnected calling of done by putting it right next to the related code:

  forEach(files, function(file) {
      emitter.once('done', this.async());
      renderFile(file, options, emitter);  // Note you were missing a semicolon here :)
    },

I think you will still need the !options.directory line in the other event handler, but it gets rid of the need for the "global".

Show outdated Hide outdated bin/node-sass
@@ -162,8 +207,7 @@ function getOptions(args, options) {
*/
function watch(options, emitter) {
var glob = options.recursive ? '**/*.{sass,scss}' : '*.{sass,scss}';
var src = isSassFile(options.src) ? options.src : path.join(options.src, glob);
var src = isSassFile(options.src) ? options.src : path.join(options.src, globPattern(options));

This comment has been minimized.

@wesleytodd

wesleytodd Apr 6, 2015

Contributor

I might be wrong here, but doesn't your options.directory solve the same problem as this isSassFile call? Maybe you can just use that here, and replace the second options.src with options.directory. Also you can replace path.resolve(path.dirname(src)) on the line below if we already know we have a directory.

@wesleytodd

wesleytodd Apr 6, 2015

Contributor

I might be wrong here, but doesn't your options.directory solve the same problem as this isSassFile call? Maybe you can just use that here, and replace the second options.src with options.directory. Also you can replace path.resolve(path.dirname(src)) on the line below if we already know we have a directory.

This comment has been minimized.

@paulcpederson

paulcpederson Apr 6, 2015

Contributor

I think you are correct. let me look.

@paulcpederson

paulcpederson Apr 6, 2015

Contributor

I think you are correct. let me look.

@paulcpederson

This comment has been minimized.

Show comment
Hide comment
@paulcpederson

paulcpederson Apr 6, 2015

Contributor

@wesleytodd good idea. let me try that out now.

Contributor

paulcpederson commented Apr 6, 2015

@wesleytodd good idea. let me try that out now.

@wesleytodd

This comment has been minimized.

Show comment
Hide comment
@wesleytodd

wesleytodd Apr 6, 2015

Contributor

Sorry for the load of comments.. Sometimes I feel like half my real job is peer review....my coworkers probably hate me :)

Seriously tho, this is a good PR. I will most definitely use this feature once it is merged and working. So thanks for the hard work!

@am11 & whoever else cares, I hope you don't mind my pitching in like this. Let me know if I am going overboard as I really haven't been involved in the development of this project much before. But I guess that is the deal with OSS right? Anyone can say whatever they want....

Contributor

wesleytodd commented Apr 6, 2015

Sorry for the load of comments.. Sometimes I feel like half my real job is peer review....my coworkers probably hate me :)

Seriously tho, this is a good PR. I will most definitely use this feature once it is merged and working. So thanks for the hard work!

@am11 & whoever else cares, I hope you don't mind my pitching in like this. Let me know if I am going overboard as I really haven't been involved in the development of this project much before. But I guess that is the deal with OSS right? Anyone can say whatever they want....

@paulcpederson

This comment has been minimized.

Show comment
Hide comment
@paulcpederson

paulcpederson Apr 6, 2015

Contributor

I love getting comments like this. I feel like they make me a better developer! 😸

Hold off on merging this though. I am finding some problems using this with the watcher. Fixing them now.

Contributor

paulcpederson commented Apr 6, 2015

I love getting comments like this. I feel like they make me a better developer! 😸

Hold off on merging this though. I am finding some problems using this with the watcher. Fixing them now.

@am11

This comment has been minimized.

Show comment
Hide comment
@am11

am11 Apr 6, 2015

Member

@wesleytodd, you are not stepping toes at all, but the opposite; a valued contribution! Feel free to continue.. :)

Member

am11 commented Apr 6, 2015

@wesleytodd, you are not stepping toes at all, but the opposite; a valued contribution! Feel free to continue.. :)

@wesleytodd

This comment has been minimized.

Show comment
Hide comment
@wesleytodd

wesleytodd Apr 6, 2015

Contributor

@am11 @paulcpederson Glad to hear it! Peer reviews are like the way I have learned the most in my career.

Contributor

wesleytodd commented Apr 6, 2015

@am11 @paulcpederson Glad to hear it! Peer reviews are like the way I have learned the most in my career.

@paulcpederson

This comment has been minimized.

Show comment
Hide comment
@paulcpederson

paulcpederson Apr 6, 2015

Contributor

@wesleytodd ok! updated everything. emitter.once was definitely the way to go. That let me clean up a lot of stuff!

Contributor

paulcpederson commented Apr 6, 2015

@wesleytodd ok! updated everything. emitter.once was definitely the way to go. That let me clean up a lot of stuff!

@wesleytodd

This comment has been minimized.

Show comment
Hide comment
@wesleytodd

wesleytodd Apr 6, 2015

Contributor

Looks good to me!

Contributor

wesleytodd commented Apr 6, 2015

Looks good to me!

}
if (!isDirectory(options.output)) {
emitter.emit('error', util.format('Multi-file compilation: requires destination %s to be a directory.', options.output));
}

This comment has been minimized.

@am11

am11 Apr 7, 2015

Member

Can we add tests for these two conditions?
For example: cli.js#L438-L448.

@am11

am11 Apr 7, 2015

Member

Can we add tests for these two conditions?
For example: cli.js#L438-L448.

This comment has been minimized.

@paulcpederson

paulcpederson Apr 7, 2015

Contributor

Good idea.

@paulcpederson

paulcpederson Apr 7, 2015

Contributor

Good idea.

});
});
it('should error if no output directory is provided', function(done) {

This comment has been minimized.

@paulcpederson

paulcpederson Apr 7, 2015

Contributor

@am11 ok, those errors are tested now.

@paulcpederson

paulcpederson Apr 7, 2015

Contributor

@am11 ok, those errors are tested now.

@paulcpederson

This comment has been minimized.

Show comment
Hide comment
@paulcpederson

paulcpederson Apr 7, 2015

Contributor

@am11 ok. I managed to test for the error message. The data was coming out of stderr as a buffer, which included \n newlines. So I had to convert it to a string and test for the error message as a substring. Maybe I'm doing something wrong?

Contributor

paulcpederson commented Apr 7, 2015

@am11 ok. I managed to test for the error message. The data was coming out of stderr as a buffer, which included \n newlines. So I had to convert it to a string and test for the error message as a substring. Maybe I'm doing something wrong?

@paulcpederson

This comment has been minimized.

Show comment
Hide comment
@paulcpederson

paulcpederson Apr 7, 2015

Contributor

They do test for the message now, though, and the tests pass.

Contributor

paulcpederson commented Apr 7, 2015

They do test for the message now, though, and the tests pass.

Show outdated Hide outdated test/cli.js
var msg = util.format('Multi-file compilation: requires destination %s to be a directory.', dest);
bin.stderr.once('data', function(data) {
assert(data.toString('utf8').indexOf(msg) > -1);

This comment has been minimized.

@am11

am11 Apr 7, 2015

Member

Probably something like this:

bin.stderr.setEncoding('utf8');
bin.stderr.once('data', function(data) {
  assert.equal(data, msg);
  done();
});
@am11

am11 Apr 7, 2015

Member

Probably something like this:

bin.stderr.setEncoding('utf8');
bin.stderr.once('data', function(data) {
  assert.equal(data, msg);
  done();
});

This comment has been minimized.

@wesleytodd

wesleytodd Apr 7, 2015

Contributor

This is actually a really brittle test. Usually when I see myself doing something like this I try to take a step back and ask, "what am I really testing?" Here the answer is that an error occurred. We don't care what the error messages was in a strict sense.

With that in mind, it might be better to test that there WAS output, and that the files were NOT compiled. That is the true test that will last longer than the next person who wants to change that error message. What do you all think about that?

@wesleytodd

wesleytodd Apr 7, 2015

Contributor

This is actually a really brittle test. Usually when I see myself doing something like this I try to take a step back and ask, "what am I really testing?" Here the answer is that an error occurred. We don't care what the error messages was in a strict sense.

With that in mind, it might be better to test that there WAS output, and that the files were NOT compiled. That is the true test that will last longer than the next person who wants to change that error message. What do you all think about that?

This comment has been minimized.

@am11

am11 Apr 7, 2015

Member

Good idea. In that case we should check whether the result (compiled target) is produced in bin.once('close', .. instead of probing bin.stderr.

@am11

am11 Apr 7, 2015

Member

Good idea. In that case we should check whether the result (compiled target) is produced in bin.once('close', .. instead of probing bin.stderr.

This comment has been minimized.

@paulcpederson

paulcpederson Apr 7, 2015

Contributor

I tend to agree with @wesleytodd . I will rewrite these to check that the compiled target doesn't exist after running and probably that it threw an error. Because if it silently just failed that would also be bad.

@paulcpederson

paulcpederson Apr 7, 2015

Contributor

I tend to agree with @wesleytodd . I will rewrite these to check that the compiled target doesn't exist after running and probably that it threw an error. Because if it silently just failed that would also be bad.

@am11

This comment has been minimized.

Show comment
Hide comment
@am11

am11 Apr 7, 2015

Member

@paulcpederson, looks pretty good. I have left a comment, probably it would be better to mark the stderr stream as utf8 prior to probing it.

Member

am11 commented Apr 7, 2015

@paulcpederson, looks pretty good. I have left a comment, probably it would be better to mark the stderr stream as utf8 prior to probing it.

@paulcpederson

This comment has been minimized.

Show comment
Hide comment
@paulcpederson

paulcpederson Apr 7, 2015

Contributor

@am11 @wesleytodd Now the tests both check for two things:

  1. Exited with an error code.
  2. No css files were generated anywhere inside the fixture.

I think that's a pretty good halfway point between checking for the exact error message and just checking there was an error at all. What do you guys think?

Contributor

paulcpederson commented Apr 7, 2015

@am11 @wesleytodd Now the tests both check for two things:

  1. Exited with an error code.
  2. No css files were generated anywhere inside the fixture.

I think that's a pretty good halfway point between checking for the exact error message and just checking there was an error at all. What do you guys think?

@wesleytodd

This comment has been minimized.

Show comment
Hide comment
@wesleytodd

wesleytodd Apr 7, 2015

Contributor

@paulcpederson Yeah, thats awesome!! This is a much more stable test.

Contributor

wesleytodd commented Apr 7, 2015

@paulcpederson Yeah, thats awesome!! This is a much more stable test.

@am11

This comment has been minimized.

Show comment
Hide comment
@am11

am11 Apr 7, 2015

Member

Excellent work @paulcpederson. 👍

Member

am11 commented Apr 7, 2015

Excellent work @paulcpederson. 👍

@am11 am11 added up-for-grabs and removed Dev - WIP labels Apr 7, 2015

am11 added a commit that referenced this pull request Apr 7, 2015

Merge pull request #838 from paulcpederson/directory
CLI: Adds support for multi-file compilation.

@am11 am11 merged commit cb9f55e into sass:master Apr 7, 2015

3 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 92.7%
Details

@am11 am11 removed the up-for-grabs label Apr 7, 2015

@paulcpederson paulcpederson referenced this pull request Apr 19, 2015

Closed

Recursive output #867

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment