Allow error messages to be reported in Sass workers #121
Conversation
This upgrades the rules_nodejs version to 2.0.1, enabling us to make use of the `run_node` functions provided there. The sass_wrapper.js is modified to directly translate argv into object arguments that are passed to the public API that is part of `sass`. Minimist is also added as a dependency.
BobobUnicorn
requested review from
davidzchen,
jelbourn and
nex3
as code owners
|
|
||
| Args: | ||
| ctx: The Bazel build context | ||
|
|
|
|
||
| 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.
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.
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.
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.
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.
This upgrades the rules_nodejs version to 2.0.1, enabling us to make use
of the
run_nodefunctions provided there.The sass_wrapper.js is modified to directly translate argv into object
arguments that are passed to the public API that is part of
sass.Minimist is also added as a dependency.
Fixes #96.