Skip to content
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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Aug 26, 2020
Merged

More error handling in the polling client #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0ca5e10
handle network and http errors in the polling client
OmarWKH Aug 21, 2020
530e992
handle parsing error and restructure error handlers
OmarWKH Aug 21, 2020
1163e25
show error on invalid JSON response and stop polling
OmarWKH Aug 21, 2020
ab1d3ec
resturecture error handling in websocket
OmarWKH Aug 21, 2020
b019752
document error handling customization in README
OmarWKH Aug 21, 2020
cf44f9e
use hasOwnProperty to check data.result exists
OmarWKH Aug 23, 2020
1b141a1
fix README documentation of error handlers
OmarWKH Aug 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,11 @@ The `initProgressBar` function takes an optional object of options. The followin
| resultElement | Override the *element* used for the result. If specified, resultElementId will be ignored. | document.getElementById(resultElementId) |
| onProgress | function to call when progress is updated | CeleryProgressBar.onProgressDefault |
| onSuccess | function to call when progress successfully completes | CeleryProgressBar.onSuccessDefault |
| onError | function to call when progress completes with an error | CeleryProgressBar.onErrorDefault |
| onError | function to call on a known error with no specified handler | CeleryProgressBar.onErrorDefault |
| onTaskError | function to call when progress completes with an error | onError |
| onNetworkError | function to call on a network error (ignored by WebSocket) | onError |
| onHttpError | function to call on a non-200 response (ignored by WebSocket) | onError |
| onDataError | function to call on a 200 response that's not JSON or has invalid schema due to a programming error (ignored by WebSocket) | onError |
| onResult | function to call when returned non empty result | CeleryProgressBar.onResultDefault |


Expand Down
64 changes: 45 additions & 19 deletions celery_progress/static/celery_progress/celery_progress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ var CeleryProgressBar = (function () {
}
}

function onErrorDefault(progressBarElement, progressBarMessageElement, excMessage) {
/**
* Default handler for all errors.
* @param data - A Response object for HTTP errors, undefined for other errors
*/
function onErrorDefault(progressBarElement, progressBarMessageElement, excMessage, data) {
progressBarElement.style.backgroundColor = '#dc4f63';
progressBarMessageElement.innerHTML = "Uh-Oh, something went wrong! " + excMessage;
}
Expand All @@ -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) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks for updating this

options = options || {};
var progressBarId = options.progressBarId || 'progress-bar';
var progressBarMessage = options.progressBarMessageId || 'progress-bar-message';
Expand All @@ -31,31 +35,53 @@ var CeleryProgressBar = (function () {
var onProgress = options.onProgress || onProgressDefault;
var onSuccess = options.onSuccess || onSuccessDefault;
var onError = options.onError || onErrorDefault;
var onTaskError = options.onTaskError || onError;
var onNetworkError = options.onNetworkError || onError;
var onHttpError = options.onHttpError || onError;
var onDataError = options.onDataError || onError;
var pollInterval = options.pollInterval || 500;
var resultElementId = options.resultElementId || 'celery-result';
var resultElement = options.resultElement || document.getElementById(resultElementId);
var onResult = options.onResult || onResultDefault;


fetch(progressUrl).then(function(response) {
response.json().then(function(data) {
if (data.progress) {
onProgress(progressBarElement, progressBarMessageElement, data.progress);
}
if (!data.complete) {
setTimeout(updateProgress, pollInterval, progressUrl, options);
let response;
try {
response = await fetch(progressUrl);
} catch (networkError) {
onNetworkError(progressBarElement, progressBarMessageElement, "Network Error");
throw networkError;
Copy link
Owner

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Show a helpful exception in the browser console.
  2. Stop executing the rest of the function.

This is the console if I throw networkError:
throwNetwork

If I don't throw networkError:
noThrowNetwork

If I throw parsingError:
throwParsing

If I don't throw parsingError:
noThrowParsing

If we don't want to throw, we could either:

  • check that response and data 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

}

if (response.status === 200) {
let data;
try {
data = await response.json();
} catch (parsingError) {
onDataError(progressBarElement, progressBarMessageElement, "Parsing Error")
throw parsingError;
}

if (data.progress) {
onProgress(progressBarElement, progressBarMessageElement, data.progress);
}
if (data.complete === false) {
setTimeout(updateProgress, pollInterval, progressUrl, options);
} else {
if (data.success === true) {
onSuccess(progressBarElement, progressBarMessageElement, data.result);
} else if (data.success === false) {
onTaskError(progressBarElement, progressBarMessageElement, data.result);
} else {
if (data.success) {
onSuccess(progressBarElement, progressBarMessageElement, data.result);
} else {
onError(progressBarElement, progressBarMessageElement, data.result);
}
if (data.result) {
onResult(resultElement, data.result);
}
onDataError(progressBarElement, progressBarMessageElement, "Data Error");
}
if (data.hasOwnProperty('result')) {
onResult(resultElement, data.result);
}
});
});
}
} else {
onHttpError(progressBarElement, progressBarMessageElement, "HTTP Code " + response.status, response);
}
}
return {
onSuccessDefault: onSuccessDefault,
Expand Down
9 changes: 5 additions & 4 deletions celery_progress/static/celery_progress/websockets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ var CeleryWebSocketProgressBar = (function () {
CeleryProgressBar.onResultDefault(resultElement, result);
}

function onErrorDefault(progressBarElement, progressBarMessageElement, excMessage) {
CeleryProgressBar.onErrorDefault(progressBarElement, progressBarMessageElement, excMessage);
function onErrorDefault(progressBarElement, progressBarMessageElement, excMessage, data) {
CeleryProgressBar.onErrorDefault(progressBarElement, progressBarMessageElement, excMessage, data);
Copy link
Owner

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.

}

function onProgressDefault(progressBarElement, progressBarMessageElement, progress) {
Expand All @@ -24,6 +24,7 @@ var CeleryWebSocketProgressBar = (function () {
var onProgress = options.onProgress || onProgressDefault;
var onSuccess = options.onSuccess || onSuccessDefault;
var onError = options.onError || onErrorDefault;
var onTaskError = options.onTaskError || onError;
var resultElementId = options.resultElementId || 'celery-result';
var resultElement = options.resultElement || document.getElementById(resultElementId);
var onResult = options.onResult || onResultDefault;
Expand All @@ -45,9 +46,9 @@ var CeleryWebSocketProgressBar = (function () {
if (data.success) {
onSuccess(progressBarElement, progressBarMessageElement, data.result);
} else {
onError(progressBarElement, progressBarMessageElement, data.result);
onTaskError(progressBarElement, progressBarMessageElement, data.result);
}
if (data.result) {
if (data.hasOwnProperty('result')) {
onResult(resultElement, data.result);
}
ProgressSocket.close();
Expand Down