Skip to content
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

Errors not properly reported in worker mode #96

Open
devversion opened this issue Sep 16, 2019 · 5 comments
Open

Errors not properly reported in worker mode #96

devversion opened this issue Sep 16, 2019 · 5 comments

Comments

@devversion
Copy link
Contributor

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.

@jelbourn
Copy link
Collaborator

@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?

@nex3
Copy link
Collaborator

nex3 commented Sep 23, 2019

According to dart-lang/sdk#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.

@jelbourn
Copy link
Collaborator

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

devversion added a commit to devversion/dart-sass that referenced this issue Jan 15, 2020
Currently the `run_` method, exposed through the `dart.sass.js` file,
returns a `Future`. The `Future` object is not accessible due to the
Dart compilation process and `Future` not being a thing in the
JavaScript/NodeJS platform.

In order to provide a better interface to the Sass API for
JavaScript/NodeJS consumers, a native `Promise` is now returned.

Fixes bazelbuild/rules_sass#96.
devversion added a commit to devversion/dart-sass that referenced this issue Jan 15, 2020
Currently the `run_` method, exposed through the `dart.sass.js` file,
returns a `Future`. The `Future` object is not accessible due to the
Dart compilation process and `Future` not being a thing in the
JavaScript/NodeJS platform.

In order to provide a better interface to the Sass API for
JavaScript/NodeJS consumers, a native `Promise` is now returned.

Fixes bazelbuild/rules_sass#96.
devversion added a commit to devversion/material2 that referenced this issue Jan 21, 2020
Updates `rules_sass` to the latest version that contains:

bazelbuild/rules_sass@dd0079b.

This fixes that errors are reported silently and only shown
once the CSS file is served in the browser. It still means that
the message might not be printed in the terminal, since there
is still bazelbuild/rules_sass#96, but
it's better seeing that something failed at compilation time.
jelbourn pushed a commit to angular/components that referenced this issue Jan 22, 2020
Updates `rules_sass` to the latest version that contains:

bazelbuild/rules_sass@dd0079b.

This fixes that errors are reported silently and only shown
once the CSS file is served in the browser. It still means that
the message might not be printed in the terminal, since there
is still bazelbuild/rules_sass#96, but
it's better seeing that something failed at compilation time.
@marcus-sa
Copy link

marcus-sa commented Jan 29, 2020

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';
}

yifange pushed a commit to yifange/components that referenced this issue Jan 30, 2020
Updates `rules_sass` to the latest version that contains:

bazelbuild/rules_sass@dd0079b.

This fixes that errors are reported silently and only shown
once the CSS file is served in the browser. It still means that
the message might not be printed in the terminal, since there
is still bazelbuild/rules_sass#96, but
it's better seeing that something failed at compilation time.
@siberex
Copy link

siberex commented Feb 19, 2020

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants