0

The make-promises-safe package changes Node.js's default behavior with regards to errors thrown in promises. Normally, in Node, these unhandled promise rejections will be logged, but a program keeps on running. With make-promises-safe installed, Node.js will exit when it encounters an unhandled promise rejection. The "safe" here means that your program won't have secret unhandled rejections, since unhandled rejections often line up with resources that have not been properly cleaned up, and these non-cleaned up resources can cause problems in a long running program.

All that I understand. However, this module comes with a warning

It is important that this module is only used in top-level program code, not in reusable modules!

The purpose of this warning is unclear. Why is it that the module authors advise folks against using this module in their own reusable modules?

Mark Rotteveel
  • 100,966
  • 191
  • 140
  • 197
Alana Storm
  • 164,128
  • 91
  • 395
  • 599
  • because it will apply `process.on(event, function (err) {` n * reusable module, did you look at the code its only 15 lines – Lawrence Cherone Sep 07 '20 at 15:45
  • @LawrenceCherone I did look at the code Lawrence. I see that as soon as make-promises-safe.js is loaded it will add that event handler to the global process object. Why is that bad in a module? It's my understanding that a Node.js CommonJS modules like make-promises-safe will load themselves once and only once, so even if make-promises-safe is loaded outside of the main program it's still only added to the global process once. I don't see what's bad about this and can speculatively think of multiple things that might be bad. What am I missing? – Alana Storm Sep 07 '20 at 16:01
  • Yeah, your right it's cached, so I would think they mean a vendor module not a local one which you have control over, if you're overwriting the logger or abort value etc in a vendor/reusable package then its first to call sets it also if its borking there is no way to fix it because the process has already been killed (tested). Might be worth opening an issue and asking why just to confirm. – Lawrence Cherone Sep 07 '20 at 16:30
  • a little test which shows modules if borking cant be caught, https://github.com/lcherone/mps-test – Lawrence Cherone Sep 07 '20 at 16:37

1 Answers1

1

I think the warning could indeed use some additional clarification if only to clarify the use of the slightly confusing terms of reusable modules and top-level program code here.

When I read the warning I felt like it was warning against using it in packages/modules you publish to npm. When a user imports your npm package (which might be totally unrelated to error-handling) in which you required the make-promises-safe package, this would implicitly impose an error-handling mechanism the user might not be aware of. You could add this to your README file of course but not everybody reads those thoroughly.

As you discussed in the comment section of your question already, the source code shows that it subscribes to the unhandledRejection event but even though it might not be as clean to require the make-promises-safe multiple times, the way it is declared, the module caching should indeed prevent the binding from happening more than once. So I would not count that as an issue. On the other hand, if every module started requiring make-promises-safe of course, there would be multiple subscriptions to the event.

So, conclusion. I would only require make-promises-safe in the entry file of your node application (fe. app.js/server.js where your register create/configure your http server for a node web application), so, code you have control over yourself as the developer. I would not require it in any node module (be it locally or publicly on npm) and leave it to the user that implements your package how to handle errors in HIS application.

stvn
  • 1,148
  • 1
  • 8
  • 24
  • 1
    The warning was, indeed, about imposing this behavior on a user unexpectedly. https://github.com/mcollina/make-promises-safe/issues/17 – Alana Storm Sep 08 '20 at 13:35