rules_sass icon indicating copy to clipboard operation
rules_sass copied to clipboard

Errors not properly reported in worker mode

Open devversion opened this issue 6 years ago • 5 comments

If a sass compilation fails within a worker process, the failure is not reported back through the the worker protocol. This is because the asynchronous call to sass.run_ is not properly awaited by @bazel/worker.

This seems to be because sass.run_ does not implement a JavaScript promise, but rather uses the Future_ construct (which seems to be a thing in the converted JS code only).

The Future_ has the following properties but I'm not sure if it would make sense to rely on one of those (no type-safety).

_Future {
  'then$1$2$onError': [Function: then$1$2$onError],
  'then$1$1': [Function: then$1$1],
  'then$1': [Function: then$1],
  '_thenAwait$1$2': [Function: _thenAwait$1$2],
  'whenComplete$1': [Function: whenComplete$1],
  '_addListener$1': [Function: _addListener$1],
  '_prependListeners$1': [Function: _prependListeners$1],
  '_removeListeners$0': [Function: _removeListeners$0],
  '_reverseListeners$1': [Function: _reverseListeners$1],
  '_complete$1': [Function: _complete$1],
  '_completeWithValue$1': [Function: _completeWithValue$1],
  '_completeError$2':
   { [Function: _completeError$2]
     '$callName': 'call$2',
     '$requiredArgCount': 1,
     '$defaultValues': [Function],
     '$stubName': '_completeError$2' },
  '_completeError$1': { [Function: _completeError$1] '$callName': 'call$1' },
  '_asyncComplete$1': [Function: _asyncComplete$1],
  '_chainFuture$1': [Function: _chainFuture$1],
  '_asyncCompleteError$2': [Function: _asyncCompleteError$2],
  '$isFuture': 1,
  'get$_completeError': [Function: tearOff__completeError$26],
  constructor: [Function: _Future],
  '$is_Future': [Function: _Future] }

I'm not too familiar with Dart, but just wanted to submit this issue. Happy to submit a PR if we have a solution (even it is just is using then$1 to create a JS promise.

devversion avatar Sep 16 '19 16:09 devversion

@nex3 I'm not at all familiar with dart2js; is there a way to make it emit a real Promise, or would one need to manually handle the Future coming out?

jelbourn avatar Sep 16 '19 16:09 jelbourn

According to https://github.com/dart-lang/sdk/issues/27315, the Dart team seems to have some plans (maybe "vague desires" is more accurate) to have Futures be valid Promises, but I strongly doubt that will happen soon enough to be useful here if it happens at all.

Currently run_ is just set to Dart Sass's normal main() function—we could work around this by setting it instead to a wrapper around main() that manually feeds the result of the Future through a Promise.

nex3 avatar Sep 23 '19 12:09 nex3

That seems straightforward enough- seems like something @alexeagle or @josephperrott might take a look into on our side

jelbourn avatar Sep 24 '19 00:09 jelbourn

Is this on the roadmap to be fixed? There are no errors being reported, and when I look at the generated CSS it actually contains the errors instead:

/* Error: expected ";".
 *   ,
 * 3 | @include cdk-overlay
 *   |                     ^
 *   '
 *   src/ui/tooltip/cs-tooltip.component.scss 3:21  root stylesheet */

body::before {
  font-family: "Source Code Pro", "SF Mono", Monaco, Inconsolata, "Fira Mono",
      "Droid Sans Mono", monospace, monospace;
  white-space: pre;
  display: block;
  padding: 1em;
  margin-bottom: 1em;
  border-bottom: 2px solid black;
  content: 'Error: expected ";".\a   \2577 \a 3 \2502  @include cdk-overlay\a   \2502                      ^\a   \2575 \a   src/ui/tooltip/cs-tooltip.component.scss 3:21  root stylesheet';
}

marcus-sa avatar Jan 29 '20 10:01 marcus-sa

Possible temporary workaround is to disable worker mode with --strategy=SassCompiler=sandboxed or --strategy=SassCompiler=local option — at least you will see errors in the console output. But local could produce other issues (I got one complex build failing with strategy set to local for rules_sass while succeeding when it is set to sandboxed or worker)

siberex avatar Feb 19 '20 14:02 siberex