search

crisp-im / node-crisp-api

:zap: Crisp API Node Wrapper
https://docs.crisp.chat/guides/rest-api/
MIT License
99 stars 38 forks source link

_request method does not reject with an Error object #46

Closed PastaJ36 closed 2 years ago

PastaJ36 commented 2 years ago

This method rejects the promise with a custom object: https://github.com/crisp-im/node-crisp-api/blob/1e6b94476621e7f3a435ae09370c8694955f6a7a/lib/crisp.js#L613-L625

Usually rejects are supplied with an Error object, so it includes stack information: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/reject#description

I don't think it is a requirement for promise rejects to use an Error instance, but it can be expected by some frameworks.

Our use case is that we just have async function errors pass up to our error monitoring service Sentry. We fixed it now with a custom wrapper that creates an error object when the promise rejects.

I'm not sure if the information in the rejects object is used somewhere else, haven't dug too deep in the code. If you need this extra info you could consider extending the Error object.

valeriansaliou commented 2 years ago

Hello! This rejection object is created on purpose, as a way to ensure all rejections contain a standardized REST API-like error data structure. I'm aware that it's usually a good practice to pass along Error objects, however in this specific case the library passes along rejection body data from the API for all non-2xx HTTP status codes for successful API requests. Therefore, for the requests to the API that fail (eg. TCP connection reset, network error, etc.), we need to instantiate a mocked API error object.

PastaJ36 commented 2 years ago

Thanks for the quick response, it's the reason we love using Crisp!

Could you elaborate a bit on on why you would need mocked API error objects and why that is bad? The additional rejection body context data is nice, but with async function usage there is not really a nice way to make use of it. This is more of a flaw with JS error handling to be fair.

By the way, I also notice the catch return a resolved promise, not sure if that's intentional. I'm not familiar with Got: https://github.com/crisp-im/node-crisp-api/blob/1e6b94476621e7f3a435ae09370c8694955f6a7a/lib/crisp.js#L586

valeriansaliou commented 2 years ago

If you look in the next Promise block, you can see further processing is done, and this becomes a reject() in the end. The library outputs are therefore consistent.