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

Compilation crashes node from within Promise #2075

Closed
perry-mitchell opened this issue Aug 27, 2023 · 9 comments
Closed

Compilation crashes node from within Promise #2075

perry-mitchell opened this issue Aug 27, 2023 · 9 comments

Comments

@perry-mitchell
Copy link

perry-mitchell commented Aug 27, 2023

Hi! I'm seeing some weird behaviour in my app that uses sass to compile styles in-app. When compilation catches some invalid SASS syntax, it not only throws an error, but it crashes the entire process altogether:

`import sass from 'sass'` is deprecated.                                                                                 │
Please use `import * as sass from 'sass'` instead.                                                                       │
node:internal/process/promises:288                                                                                       │
            triggerUncaughtException(err, true /* fromPromise */);                                                       │
            ^                                                                                                            │
                                                                                                                         │
sass.Exception [Error]: Undefined operation ""-16px" * 2".                                                               │
   ╷                                                                                                                     │
41 │             height: calc(100% - #{$vb-footer-vert-pad * 2})                                                         │
   │                                   ^^^^^^^^^^^^^^^^^^^^^^^                                                           │
   ╵                                                                                                                     │
  ../tmp/tmp-14-SluKh9VGcLx5/_data_resources_styles_visual_app_header_footer.sass 41:35  @import                         │
  ../tmp/tmp-14-SluKh9VGcLx5/resources/styles/visual/index.sass 11:9                     @import                         │
  ../tmp/tmp-14-SluKh9VGcLx5/__entry.sass 6:9                                            root stylesheet                 │
    at Object.wrapException (/data/node_modules/sass/sass.dart.js:1252:43)                                               │
    at Object.throwExpression (/data/node_modules/sass/sass.dart.js:1271:15)                                             │
    at SassString0.times$1 (/data/node_modules/sass/sass.dart.js:111900:16)                                              │
    at _EvaluateVisitor_visitBinaryOperationExpression_closure1.call$0 (/data/node_modules/sass/sass.dart.js:93664:21)   │
    at _EvaluateVisitor1._evaluate0$_addExceptionSpan$1$3$addStackFrame (/data/node_modules/sass/sass.dart.js:92449:23)  │
    at _EvaluateVisitor1._evaluate0$_addExceptionSpan$2 (/data/node_modules/sass/sass.dart.js:92463:19)                  │
    at _EvaluateVisitor1.visitBinaryOperationExpression$1 (/data/node_modules/sass/sass.dart.js:91458:19)                │
    at BinaryOperationExpression0.accept$1$1 (/data/node_modules/sass/sass.dart.js:86771:22)                             │
    at BinaryOperationExpression0.accept$1 (/data/node_modules/sass/sass.dart.js:86774:19)                               │
    at _EvaluateVisitor1._evaluate0$_performInterpolationHelper$3$sourceMap$warnForColor (/data/node_modules/sass/sass.d │
│ art.js:92265:24)                                                                                                                                                     │
                                                                                                                         │
Node.js v18.17.1

This is my basic usage:

import sass from "sass";

async function renderStyles(
    // <snip>
): Promise<string> {
    // <snip>
    await fs.writeFile(entry, finalOverrides);
    // Sync as it's almost 2x as fast as async SASS compilation
    const { css: compiled } = sass.compile(entry, {
        logger: sass.Logger.silent,
        quietDeps: true
    });
    // <snip>
    return compiled;
}

Any idea what I'm doing wrong? This promise has a catch block which catches other errors before this point, but if SASS compilation fails node dies like you see above.

I'm using the latest sass version:

$ npm ls sass
<snip>@2.0.0-r.266 /<snip>
├─┬ [email protected]
│ └── [email protected] deduped
└── [email protected]

EDIT: I wonder if the compileAsync mode would help with this, but then the documentation makes that sound like a poor idea:

image
@jathak
Copy link
Member

jathak commented Aug 28, 2023

Can you provide a minimal reproduction, either as a repo or just the Sass code that you're compiling with the JS code above?

@perry-mitchell
Copy link
Author

I'll try to set one up this week.

Is there any chance that there was some kind of bug in an earlier release that caused uncaught errors in compile to propagate to the top process? I've since upgraded and haven't seen the error since.. but I'll see if I can reproduce with the older version first.

@stof
Copy link
Contributor

stof commented Sep 21, 2023

the error message looks like a bug in your Sass file, where $vb-footer-vert-pad would be a string containing 16px and not a number.

@perry-mitchell
Copy link
Author

the error message looks like a bug in your Sass file, where $vb-footer-vert-pad would be a string containing 16px and not a number.

Yes, but it shouldn't crash an async compile call. It's just by example here, to demonstrate the issue.

@stof
Copy link
Contributor

stof commented Sep 21, 2023

It does not crash the call. It rejects the promise, which is the expected way to return errors.

As you don't show how you call your renderStyles async function, we cannot know whether you properly handle errors. But my guess is that the answer is no if you end up in the triggerUncaughtException part of node.

@perry-mitchell
Copy link
Author

perry-mitchell commented Sep 21, 2023

It does not crash the call. It rejects the promise, which is the expected way to return errors.

Your assumption is incorrect I'm afraid - I can assure you it does, as I witnessed it many times. Note the following:

sass.Exception [Error]: Undefined operation ""-16px" * 2".

Node.js v18.17.1

The last line shows as Node's process dies. The renderStyles function is called as follows:

const styles = await renderStyles(sassFilesIndex, type, themePrefix);

And this is called from within an async express controller. All controllers are wrapped in express-promise-router, and fall back to a forced error handler so all async exceptions are caught. This has remained the same in this project since day one and I've not had a single crash due to uncaught promise exceptions. For some reason awaiting this renderStyles call, in which I call the synchronous compile styles, it hard crashes and is not caught by the same async handler I just mentioned:

    const { css: compiled } = sass.compile(entry, {
        logger: sass.Logger.silent,
        quietDeps: true
    });

I would expect sass sync calls, when throwing exceptions, to still be caught by the same async handlers as any other normal call would be. Is it possible that something in the dart version, in sync mode, is causing some error that cannot be caught by the Javascript async logic?

EDIT: I'll do my best to create a reproduction tonight/tomorrow for this.

@stof
Copy link
Contributor

stof commented Sep 21, 2023

None of the code snippets you shared until now show an async error handler.

@perry-mitchell
Copy link
Author

perry-mitchell commented Sep 21, 2023

A lot of it is internal code in my company, which is why I haven't shown it. I've explained that it's wrapped in express-promise-router, which does provide all-encompassing async error handling.

Again, I'll try to get a reproduction added here shortly. It'd be great to keep the comments constructive..

@perry-mitchell
Copy link
Author

Haven't had time to test or reproduce, and it hasn't been an issue lately. I'll reopen if I come across it again, and am able to create a reproduction.

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

No branches or pull requests

3 participants