use npm-sass to find bin #3
Conversation
|
|
||
| // jscs:disable maximumLineLength | ||
| module.exports = { | ||
| 'make-folders': `mkdir -p ${target}/js ${target}/css ${target}/images`, | ||
| 'compile-css': `${sass} ${source}/scss/app.scss ${target}/css/app.css --include-path ./node_modules`, | ||
| 'compile-css': `${npmsass} ${source}/scss/app.scss > ${target}/css/app.css`, |
There was a problem hiding this comment.
This doesn't seem right. You're effectively toString-ing the module to get the string npm-sass. Just use the string.
There was a problem hiding this comment.
In fact, since this isn't an npm script, you'll need the fully qualified path to the binary, so I'd leave as it was with witch finding the binary path.
|
|
||
| // jscs:disable maximumLineLength | ||
| module.exports = { | ||
| 'make-folders': `mkdir -p ${target}/js ${target}/css ${target}/images`, | ||
| 'compile-css': `${sass} ${source}/scss/app.scss ${target}/css/app.css --include-path ./node_modules`, | ||
| 'compile-css': `${npmsass} ${source}/scss/app.scss > ${target}/css/app.css`, |
There was a problem hiding this comment.
What are you doing here? isn't npmsass the actual module? Are these my hands?
- update node-sass task to use npm-sass
JoeChapman
force-pushed
the
fix/npm-sass
branch
from
306d82f to
b25969c
Compare
| @@ -4,12 +4,12 @@ const path = require('path'); | |||
| const cwd = process.cwd(); | |||
| const target = path.resolve(cwd, 'public'); | |||
| const source = path.resolve(__dirname, '../src'); | |||
| const sass = require('witch')('node-sass'); | |||
| const npmsass = require('npm-sass'); | |||
There was a problem hiding this comment.
we should either use npmsass programmatically or just use the name of the binary
There was a problem hiding this comment.
personally, I would prefer all of this to be in just plain ol boring JS code, include the modules, use them, etcetera. But I know you love your dirty bash
There was a problem hiding this comment.
o yeah I don't think you need this. I did it without it
There was a problem hiding this comment.
personally, I would prefer all of this to be in just plain ol boring JS code
I'm inclined to agree, not least because there's absolutely zero error handling code in this setup at the moment. If things fail you get a nice console.error, but the process doesn't actually fail (i.e. exits with 0)
|
@lennym @joefitter @sulthan-ahmed changes made |
|
actually @JoeChapman why are we eslintignoring bin? That dupe var would have been picked up if we linted it |
|
|
||
| // jscs:disable maximumLineLength | ||
| module.exports = { | ||
| 'make-folders': `mkdir -p ${target}/js ${target}/css ${target}/images`, | ||
| 'compile-css': `${sass} ${source}/scss/app.scss ${target}/css/app.css --include-path ./node_modules`, | ||
| 'compile-css': `npmsass ${source}/scss/app.scss > ${target}/css/app.css`, |
There was a problem hiding this comment.
You need to witch this, same as before.
There was a problem hiding this comment.
Actually, there's an outside chance you might not need to, npm might work it out since you're inside the bin from package.json, but I'd want to check that first.
witch will be safe though.
| @@ -4,12 +4,12 @@ const path = require('path'); | |||
| const cwd = process.cwd(); | |||
| const target = path.resolve(cwd, 'public'); | |||
| const source = path.resolve(__dirname, '../src'); | |||
| const sass = require('witch')('node-sass'); | |||
| const npmsass = require('npm-sass'); | |||
There was a problem hiding this comment.
personally, I would prefer all of this to be in just plain ol boring JS code
I'm inclined to agree, not least because there's absolutely zero error handling code in this setup at the moment. If things fail you get a nice console.error, but the process doesn't actually fail (i.e. exits with 0)
|
@joefitter will address the eslintignore stuff in separate pr |
I'll raise an issue for this - unless you guys would prefer it to be addressed before merging? |
|
@joefitter Makes sense. No reason at all IMO to block merge - it's the same on master, it's just something that I spotted when I was reviewing/trying to work out what was going on. |
|
@joefitter I'm not sure I follow, what is the context of that comment? |
|
@JoeChapman rather than doing: "browserify": "browserify ./target ./output --plugin ./blah"we would do: var browserify = require('browserify');
var b = browserify();
b.add('./browser/main.js');
b.bundle().pipe(process.stdout); |
|
ok, fair enough. However, I would prefer not to change all the things in this PR. Can you raise an issue so we can discuss...? |
|
@JoeChapman already did |
|
I need one more +1 @lennym @sulthan-ahmed @easternbloc @gxxm |
Use npm-sass to find install path of node-sass