-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[UI Framework] Spawn compileCss as a child process to prevent a node-sass fatal error from killing the watch process #13222
[UI Framework] Spawn compileCss as a child process to prevent a node-sass fatal error from killing the watch process #13222
Conversation
…r from killing the watch process.
package.json
Outdated
@@ -63,7 +63,8 @@ | |||
"mocha": "echo 'use `node scripts/mocha`' && false", | |||
"sterilize": "grunt sterilize", | |||
"uiFramework:start": "grunt uiFramework:start", | |||
"uiFramework:build": "grunt uiFramework:build" | |||
"uiFramework:build": "grunt uiFramework:build", | |||
"uiFramework:compileCss": "grunt uiFramework:compileCss" |
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.
Expanding on our conversation here. When would someone need to manually compile the CSS? I can understand creating a grunt task for it, but not sure it warrants an aliased npm task.
@tylersmalley I've addressed your feedback. Can you take another look? |
tasks/ui_framework.js
Outdated
grunt.file.write('ui_framework/dist/ui_framework.css.map', result.map); | ||
} | ||
}); | ||
return new Promise(resolve => { |
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.
We should use Bluebird.promisify
instead of wrapping here
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.
Could you show me what you mean?
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.
Thinking something like this:
import { promisify } from 'bluebird';
const renderSass = promisify(sass.render);
function uiFrameworkCompile() {
const from = 'ui_framework/components/index.scss';
const to = 'ui_framework/dist/ui_framework.css';
return renderSass({
file: from
}).then(result => {
postcss([postcssConfig])
.process(result.css, { from, to })
.then(result => {
grunt.file.write(to, result.css);
if (result.map) {
grunt.file.write(`${to}.map`, result.map);
}
});
}).catch(error => {
grunt.log.error(error);
});
}
const debouncedCompile = debounce(() => { | ||
// Compile the SCSS in a separate process because node-sass throws a fatal error if it fails | ||
// to compile. | ||
grunt.util.spawn({ |
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.
If you wanted to use a promise here you could as well, but I feel the callback is fine if you want to leave it.
const spawn = promisify(grunt.util.spawn);
...
spawn({
cmd: isPlatformWindows ? '.\\node_modules\\.bin\\grunt.cmd' : './node_modules/.bin/grunt',
args: [ 'uiFramework:compileCss' ]
}).then(grunt.log.writeln).catch(e => grunt.log.error(e.stdout));
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.
One minor request to use promisify.
@tylersmalley When I implemented your change, the output in the terminal lost some of its color, and the "bell" also didn't ring when there was an error: Here's how it looked before: Any suggestions on what's going on here and how we can get this behavior back? |
e9e665c
to
ea7e67e
Compare
tasks/ui_framework.js
Outdated
|
||
return new Promise(resolve => { | ||
sass.render({ | ||
file: 'ui_framework/components/index.scss' |
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.
Can use src
here
Good catch! Fixed. Thanks. |
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.
LGTM - not sure why promisify is dropping the colors and bell. Got the same results with Node 8's util.promisify. This works fine.
…sass fatal error from killing the watch process (elastic#13222) * Spawn compileCss as a child process to prevent a node-sass fatal error from killing the watch process. * Document tasks.
…sass fatal error from killing the watch process (elastic#13222) * Spawn compileCss as a child process to prevent a node-sass fatal error from killing the watch process. * Document tasks.
No description provided.