Closed Garfonso closed 4 years ago
I ran my test from #71 against old versions and it is failing in v1.2.0 and working in >= v1.3.0.
in v1.2.0 there was no try-catch block, but the try-catch block resolves the unhandled rejection issue. The try-catch block looks correct to me and I'm not replicating the issue so I don't see the reason to need the .catch()
Maybe it is the wrong part in the code. But a project I use gets constant unhandled promise rejections. I did not yet find out where they come from (but they originate in this library, no other network connection is used and I triple checked all api calls do have a catch clause).
Indeed the PR did not completely fix the issue for me, and I, too, think the original code looks good after another inspection. I sadly did not find time, yet, to dig deeper.
If you like, you can close the PR. Just know that there is probably some issue with unhandled promise rejection.
/edit: also I am not sure I did test the latest version, yet.
Just because you're seeing an unhandled promise rejection doesn't mean this library is at fault. Most of the functions on this library return a promise. If you fail to .catch() or try-catch with await, it will be unhandled, but that was not the fault of this library. Much like the original post of #71 adding a catch() fixes that unhandled promise rejection.
This should fix #71 as discussed in the issue. Otherwise rejections on network errors are not handled which will a warning with newer node versions and might crash the process in future node versions (you can configure it to do so today already).