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

Catch rejected promises in shell scripts #671

Closed
LoicPoullain opened this issue Apr 7, 2020 · 3 comments
Closed

Catch rejected promises in shell scripts #671

LoicPoullain opened this issue Apr 7, 2020 · 3 comments

Comments

@LoicPoullain
Copy link
Member

When an error is thrown in an async main of a shell script, we get this warning:

(node:15576) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:15576) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

We can probably fix this by making run-script async in packages/cli/src/run-script/run-script.ts and wrap up main(args) with a trycatch (err) { console.log(err) }

@LoicPoullain LoicPoullain added this to Backlog in Issue tracking via automation Apr 7, 2020
@LoicPoullain LoicPoullain moved this from Backlog to To Do in Issue tracking Apr 29, 2020
@damlys
Copy link
Contributor

damlys commented May 14, 2020

Can be improved with following event handlers 👌

process.on("uncaughtException", (error: any): void => {
  process.stderr.write(`[uncaught_exception] ${error.toString()}\n`);
  process.exit(1);
});

process.on("unhandledRejection", (reason: any, promise: Promise<any>): void => {
  process.stderr.write(`[unhandled_rejection] ${reason.toString()}\n`);
  process.exit(1);
});

@LoicPoullain
Copy link
Member Author

I'd rather not to go with global event handlers because:

  • they are hard to unit test
  • and they may hide unexpected errors thrown by the function runScript and not the script itself.

I think that a try/catch is more appropriate in this case.

@LoicPoullain
Copy link
Member Author

Fixed in v1.10.0

Issue tracking automation moved this from Work In Progress to Done / Closed This Release Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Issue tracking
  
Done / Closed This Release
Development

No branches or pull requests

2 participants