-
Notifications
You must be signed in to change notification settings - Fork 381
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
Lazily require node-sass #748
Conversation
Our company also would like to see this merged as it should allow for node 12 support with the node-sass dependency. |
@dlmanning can you please check & merge this PR? |
@@ -118,6 +118,9 @@ const gulpSass = (options, sync) => through.obj((file, enc, cb) => { // eslint-d | |||
return cb(new PluginError(PLUGIN_NAME, error)); | |||
}; | |||
|
|||
// eslint-disable-next-line global-require, import/no-extraneous-dependencies | |||
const compiler = gulpSass.compiler || require('node-sass'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not this be auto-detection?
let compiler = gulpSass.compiler;
if (!compiler) {
try {
compiler = require('node-sass');
} catch (e) {
try {
compiler = require('sass');
} catch (e) {
return cb(new PluginError(
PLUGIN_NAME,
"No compiler found!\nPlease install one of the npm packages: node-sass or sass, or set compiler in code:\n const sass = require('gulp-sass');\n sass.compiler = require('sass');\n"
));
}
}
}
So that user don't need to set dart-sass by hand. For reference, webpack's sass-loader does this kind of auto-detection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot of sense, however I'm not in a position to fix this at the moment.
I'll probably get to it somewhere next week (if nobody else has yet).
npm install gulp-sass --save-dev | ||
``` | ||
You must also install either `node-sass` or `sass`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on changing this to:
You can either choose
node-sass
orsass
:npm install gulp-sass sass --save-dev
or:
npm install gulp-sass node-sass --save-dev
This way, users can copy the one-liner they prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense. Perhaps we should also add some additional information on the differences?
Can I help with anything to speed this along? Without this change, we are blocked on Node 14. |
I've just found https://www.npmjs.com/package/gulp-dart-sass that can be used as a drop-in replacement. |
Thanks all, close this to focus efforts on #802 which is more rounded at this point in time. |
Closes #747.
Implementation of #747 for your convenience.
I'm technically in violation of the contributor guidelines by opening this PR without prior approval, but I need this functionality right now and I want to use my fork in the meantime.
I'm not sure how I should update the changelog, should I add a new version "heading"?