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

global exception handler being added that causes gnarly stack traces #32

Open
rtpg opened this issue Jan 13, 2023 · 2 comments · May be fixed by #33
Open

global exception handler being added that causes gnarly stack traces #32

rtpg opened this issue Jan 13, 2023 · 2 comments · May be fixed by #33

Comments

@rtpg
Copy link

rtpg commented Jan 13, 2023

This library's emscriptem code sets up an uncaughtException listener at the top level if it is imported.

This handler will re-throw the error, causing the logging to print the exception source line as the minified single line of the project. So importing this library and then having an error will cause a big

const vizRenderSync = require("@aduh95/viz.js/sync")

function f(){
  return (undefined).a.b
}

f()

^ you can see the issue running this in node. The stack trace just dumps the entire file.

This seems to be related to emscripten-core/emscripten#17228 and this specific part of the source

From that issue:

This is probably a duplicate of emscripten-core/emscripten#11532 and emscripten-core/emscripten#14270, could you try to add -sNODEJS_CATCH_EXIT=0 -sNODEJS_CATCH_REJECTION=0 to the linker invocation? Note that this requires at least Node v15 to work properly.

So I think if the configuration adds those two parameters then this library will no longer include that exception handler.

@rtpg
Copy link
Author

rtpg commented Jan 13, 2023

I'd send in a PR for this but I honestly don't quite know where the linker step is in the Makefile so am unsure where this makes sense

@rtpg rtpg linked a pull request Jan 31, 2023 that will close this issue
@rtpg
Copy link
Author

rtpg commented Jan 31, 2023

Alright, created a PR that locally worked as expected. Would definitely appreciate a merge + a release of this change, as right now my stack traces are very hard to read without this

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

Successfully merging a pull request may close this issue.

1 participant