-
Notifications
You must be signed in to change notification settings - Fork 68
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
Allow error messages to be reported in Sass workers #121
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
{ | ||
"devDependencies": { | ||
"@bazel/worker": "latest", | ||
"minimist": "1.2.5", | ||
"sass": "1.26.10" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,11 +13,44 @@ | |
const {debug, runAsWorker, runWorkerLoop} = require('@bazel/worker'); | ||
const sass = require('sass'); | ||
const fs = require('fs'); | ||
const minimist = require('minimist'); | ||
|
||
const args = process.argv.slice(2); | ||
if (runAsWorker(args)) { | ||
debug('Starting Sass compiler persistent worker...'); | ||
runWorkerLoop(args => sass.cli_pkg_main_0_(args)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the goal of this change? It adds a lot of complexity and requires us to update multiple places any time we want to change which arguments are passed in to the compiler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's to address #96. The issue is that the Dart _Future is not directly translatable to JS promises, so the worker loop doesn't have any idea when the compilation is actually done (or if it succeeded). At present, it just treats the unknown object as "truthy". I couldn't find a way to wrap a Future in a Promise on the JS side; it may have to be done in Dart. One other option could be to try and farm out to an execFile so we can just pass the arguments directly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather fix the root cause by adding support to http://github.com/google/dart_cli_pkg for wrapping futures in Promises than make this package harder to maintain going forward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing that's google/dart_cli_pkg#50? I'm happy to take a look there; let me shelve this for now then. |
||
runWorkerLoop(args => { | ||
const argv = minimist(args); | ||
const positionalArgs = argv['_']; | ||
const output = positionalArgs[1]; | ||
const input = positionalArgs[0]; | ||
|
||
const sourceMap = | ||
typeof argv['source-map'] === 'boolean' ? argv['source-map'] : true; | ||
const embedSources = | ||
typeof argv['embed-sources'] === 'boolean' | ||
? argv['embed-sources'] | ||
: false; | ||
|
||
try { | ||
const result = sass.renderSync({ | ||
file: input, | ||
outFile: output, | ||
includePaths: argv['load-path'], | ||
outputStyle: argv['style'], | ||
sourceMap, | ||
sourceMapContents: embedSources | ||
}); | ||
|
||
fs.writeFileSync(output, result.css); | ||
if (sourceMap) { | ||
fs.writeFileSync(output + '.map', result.map); | ||
} | ||
return true; | ||
} catch (e) { | ||
console.error(e.message); | ||
return false; | ||
} | ||
}); | ||
// Note: intentionally don't process.exit() here, because runWorkerLoop | ||
// is waiting for async callbacks from node. | ||
} else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,6 +155,11 @@ long@^4.0.0: | |
resolved "https://registry.yarnpkg.com/long/-/long-4.0.0.tgz#9a7b71cfb7d361a194ea555241c92f7468d5bf28" | ||
integrity sha512-XsP+KhQif4bjX1kbuSiySJFNAehNxgLb6hPRGJ9QsUr8ajHkuXGdrHmFUTUUXhDwVX2R5bY4JNZEwbUiMhV+MA== | ||
|
||
[email protected]: | ||
version "1.2.5" | ||
resolved "https://registry.yarnpkg.com/minimist/-/minimist-1.2.5.tgz#67d66014b66a6a8aaa0c083c5fd58df4e4e97602" | ||
integrity sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw== | ||
|
||
normalize-path@^3.0.0: | ||
version "3.0.0" | ||
resolved "https://registry.yarnpkg.com/normalize-path/-/normalize-path-3.0.0.tgz#0dcd69ff23a1c9b11fd0978316644a0388216a65" | ||
|
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.
Why these cosmetic changes?