-
Notifications
You must be signed in to change notification settings - Fork 28
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
HTTP errors are not handled #82
Comments
I have a fix in progress, but will need to add a testing dependency on something like nock for mocking http responses. If you prefer another testing approach, let me know. |
Hey Tom! Thanks for looking into this! Completely missed that Your approach to check for valid response, then rejecting with an error if invalid sounds good. Using nock is fine (I've been meaning to add something like this to the library's tests for a while now anyway). Let me know if you need any help with getting a fix finished up. Would be great to have that merged in ASAP 😄 |
Hey @tgvarik! Any progress on the PR? Keen to get this bug fixed ASAP. Will fix myself if you don't have the time after all. Cheers :) |
Incoming momentarily... |
The promises returned by node-fetch are not rejected for http status codes in the 4xx or 5xx block. (According to node-fetch's readme, such errors should be handled within the
.then()
call.) node-tvdb does not check for these kind of errors before trying to parse the response body as json. This means that when the api returns a non-json body under a 4xx or 5xx error code, node-tvdb's promises are rejected with unhelpful errors, like this:NB: This is happening a lot right now, because the api is going through one of its annoyingly frequent "Cloudflare is timing out while trying to reach the origin server" phases. The api returns html under a 522 status.
Unhelpful errors make it hard for consuming code to recover gracefully. Instead, node-tvdb could check for 4xx or 5xx statuses and reject with more helpful errors that include the response object from node-fetch. The consuming code could then examine the response object to see why the call failed and respond accordingly.
The text was updated successfully, but these errors were encountered: