-
Notifications
You must be signed in to change notification settings - Fork 91
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
More error handling in the polling client #43
Conversation
- there's one default error handler - other errors (task, network, http, data) can be customized - pass the Response when handling http errors
still, only a task error is recognized by websocket currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Give me a few days to do a bit of testing and I'd also be interested to hear from @EJH2 on the websockets bit (though looks innocuous enough).
response = await fetch(progressUrl); | ||
} catch (networkError) { | ||
onNetworkError(progressBarElement, progressBarMessageElement, "Network Error"); | ||
throw networkError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the value in re-throwing this one (and the parsingError
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Show a helpful exception in the browser console.
- Stop executing the rest of the function.
This is the console if I throw networkError:
If I don't throw networkError:
If I don't throw parsingError:
If we don't want to throw, we could either:
- check that
response
anddata
are defined before using them - or
return
- or use .then/.catch callbacks instead of try/catch
- or move the "success" code inside the try block
function onErrorDefault(progressBarElement, progressBarMessageElement, excMessage) { | ||
CeleryProgressBar.onErrorDefault(progressBarElement, progressBarMessageElement, excMessage); | ||
function onErrorDefault(progressBarElement, progressBarMessageElement, excMessage, data) { | ||
CeleryProgressBar.onErrorDefault(progressBarElement, progressBarMessageElement, excMessage, data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EJH2 do you have any thoughts on this part? I also have never used the websockets client.
@@ -22,7 +26,7 @@ var CeleryProgressBar = (function () { | |||
progressBarMessageElement.innerHTML = progress.current + ' of ' + progress.total + ' processed. ' + description; | |||
} | |||
|
|||
function updateProgress (progressUrl, options) { | |||
async function updateProgress (progressUrl, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks for updating this
I found celery-progress really helpful so I'm happy to contribute. Thanks for making it :) |
Hey there, I'd be happy to look at this with the websocket implementation in a day or two. I don't expect to see any issues because the websocket functions pretty much the same as http, but I'll make sure to let you know if I find anything! |
@EJH2 I'm going to merge this for now though feel free to leave any comments if there are any issues. I'll plan to cut a new release in the next few days. |
Everything looks fine to me on the websocket side of things! |
Just released this in |
This adds more specific error handlers. As a result:
onError
becomes the default generic handlers. Users can customize task errors, network errors, HTTP errors, and data errors. I describe each of them in the README.I also replaced
if (data.result)
withif (data.hasOwnProperty('result'))
. The former fails if result is false and assumes there is no result.I tested these use cases:
data.complete
ordata.success
).3 and 4 are programming errors handled by
onDataError
. There's no good way to handle them except show the user an error and stop polling.This could address #13.
This is only for the polling client. I do not use webosckets currently, so I can't test websocket specific errors.