Add proper error handling and display

[kh90909]

This commit adds retrying and error handling for all ParticleJS API calls. In general, API calls are retried three times with 1 second delay between each. If the call still does not succeed, the error is categorized into one of three groups (MINOR, MAJOR or FATAL) which dictate the action that should be taken.

For a minor error, e.g. failure to get a variable value, the error is logged to the JS console, but not communicated to the user. For a major error, e.g. failure to send an event, an error message is printed to the terminal. For a fatal error, i.e. the failure of an API call the result of which we cannot proceed without, a modal is displayed informing the user and giving them the option to retry (which falls back into the calling retry_promise() and retries three more times) or logout. When the modal is displayed, subsequent errors are silenced, pollers are stopped and the subscribed event stream (if there is one) is aborted. Upon retry, the pollers and event stream are restarted.

This is branched off #63 as that's going to be merged first, so ignore all but the last commit. I've left lots of console logging in place for testing, but plan to remove most if not all of it before merging.

This will resolve #64.

[emcniece]

Haven't reviewed the entire commit yet, but here's a few notes so far:

• Fresh session: login screen has 2 oakterm_error_catch_at_end logs, 2 Uncaught (in promise) Missing Credential errors. Why are these messages displayed twice? Can we streamline the process by using the firstRun flag?
• Fresh session: clicking "Log In" with failing credentials (or none at all) results in API call retries with failures, ending in a modal. Is this a good response for failing login?
• retry_promise: specify majority defaults inside function scope instead of specifying in calls. Consider a boolean for negating API_retries and API_retry_delay, as this function is either called with defaults (API_retries and API_retry_delay) or (0,0).
• The retry_promise, then(thing), catch(oakterm_...) chain is very prevalent - can this be abstracted or streamlined?

Will continue review tomorrow!

[kh90909]

I'll get to the rest of the points tomorrow, but just to address these two:

• Fresh session: login screen has 2 oakterm_error_catch_at_end logs, 2 Uncaught (in promise) Missing Credential errors. Why are these messages displayed twice? Can we streamline the process by using the firstRun flag?
• Fresh session: clicking "Log In" with failing credentials (or none at all) results in API call retries with failures, ending in a modal. Is this a good response for failing login?

I stayed away from the login code when doing the new error handling because it dealt with errors differently, displaying them on the login screen. What I missed was the way those errors were being caught and the resulting propagation down the promise chain.

The commit above reworks the login code to use promise retries and the new error handling framework.

One thought prompted by this rework is that we should really only retry on certain errors. For example, a network connectivity error should be retried, but an invalid access token error is not going to change with retries. This probably isn't necessary for MVP, since the only side effect is the slight delay for the retries. We could drop the retry count to zero or one if the delay feels obtrusive.

The double oakterm_error_catch_at_end logs are by design, although they now only occur on an actual error, and I plan to remove this log message once we're happy everything is working correctly. The doubling results from the promise chain being split in two so that subscribe_events does not depend on get_variables succeeding, and so that they run in parallel.

[kh90909]

retry_promise: specify majority defaults inside function scope instead of specifying in calls.

Good idea. I'll move the defaults inside so that they don't have to be specified in every call, but I'm leaning towards keeping the default value variables defined at the top of the file so that someone could change them without having to wade through the code.

Consider a boolean for negating API_retries and API_retry_delay, as this function is either called with defaults (API_retries and API_retry_delay) or (0,0).

I'd like to keep retry_promise as generic as possible, and maybe even make it more so by removing the references to OakTerm specific attributes. So, I think the best way to specify the majority defaults is to use default parameter values (e.g., implemented as in the pre ES2015 example here), so that these parameters can still be customized as desired, but don't need to be specified otherwise. I'll put a commit together for this.

By the way, since the (0,0) calls effectively disable retries, they could actually be rewritten as

.then(async_func)
.catch(error_handler)

The reason I've kept them as they are is that I hope to enable retries in most of these cases later on. First, I need to verify that the ParticleJS publishEvent call will never publish an event but also return an error, which I haven't had a chance to do yet.

[kh90909]

• The retrypromise, then(thing), catch(oakterm...) chain is very prevalent - can this be abstracted or streamlined?

I'm not in favor of abstraction because I want to keep the promise chains as human-readable and easy to understand as possible. I like the fact that the flow of asynchronous actions is clearly spelled out like this.

But, I'm open to suggestions for streamlining. I considered renaming retry_promise to retry to shorten the lines, but was concerned about it being too generic. What do you think? I also wondered about integrating it into the Promise object with Promise.prototype.retry = .... In that case, I think I'd definitely need to rework it to be fully generic, and move the OakTerm specific stuff into handler functions.

I agree the .catch(oakterm_error_catch_at_end) is superfluous from a readability point-of-view, but I don't see a way to abstract or streamline this unless you have any thoughts? When errors occur, a rejected promise is returned to skip the rest of the chain, so we need this .catch at the end to handle the reject. Otherwise, an uncaught exception is logged to the console.

[kh90909]

Okay, I've reworked this code some more, in line with my comments above. The retry function is now fully generic and accepts a conditional retry handler so that retries can be added or cancelled. This handler can also return a promise to defer the decision.

This means the fatal error handling (the error modal, etc) can move into this conditional handler. It uses the promise return to trigger another retry when the retry button is clicked. This conditional handler also allows retries to be skipped for "definitive" errors (e.g. invalid access token) that are not going to resolve with retries.

Relocating the fatal error handling like this means the main error handler can move to the end of each promise chain, which does away with all the _catch_at_end lines.

How do you feel about extending prototypes? I see some people regard it as a no-no. I currently have the promise retry function (plus a delay function) implemented as Promise.prototype.retry, etc. because it makes the promise chains look so much neater, but we can easily switch to standalone functions.

[kh90909]

Also, here are flowcharts of the retry and handler functions that I used to get things straight before writing the code:

[kh90909]

The above charts can be viewed and edited on draw.io if required: https://www.draw.io/i/QT1rdy3

[emcniece]

Still doing UI-level testing, haven't dug into code yet - apologies.

https://rawgit.com/kh90909/OakTerm/i64-proper-error-handling/index.html

The modal no longer displays upon failed login, but the app still waits for 3 retries before showing an error message - this is too long with no response, and submissions can be stacked (login button reclicks) to galore, resulting in API floods. This might be improved by not retrying an API action like an auth... thought I saw you mention this elsewhere.

I am not particularly in favor of extending prototypes unless absolutely necessary. While a given browser may work fine with an extended prototype, the standards are continually updated and browsers will parse methods differently in the future. There is potential here for disjoint and it will require us to keep on top of testing in new browser versions despite promises of backward compatibility. There are also implementations that transpile other scripts down to ES5, so having to troubleshoot something like Babel because of an extended prototype (if for whatever reason we have to port) might be a challenge. If there is an easy way to avoid such an extension, cool - if it's WAY easier to extend a prototype, let's keep the code for now!

[kh90909]

The modal no longer displays upon failed login, but the app still waits for 3 retries before showing an error message - this is too long with no response, and submissions can be stacked (login button reclicks) to galore, resulting in API floods. This might be improved by not retrying an API action like an auth... thought I saw you mention this elsewhere.

I guess you must have tested this by putting in an invalid email address? I missed that case...didn't realize it returned a different error message to a valid email but incorrect password. Anyway, that's now added to the definitive errors list so that it won't retry.

I didn't realize the Login button wasn't getting disabled after clicking to prevent API floods - I saw you had a line re-enabling the button in login_success and login_err, but didn't realize there was no corresponding line disabling it in the first place. I added it.

For now, I've kept the retries on the login attempt for errors we have not defined as definitive. My opinion is that we should retry in the case of errors like network connectivity, etc. But, I don't feel strongly about it so I'm happy to disable them (or change the number or interval) if you think it's better. It's just a one line change.

Finally, those are fair points about extending prototypes, so I moved these to standalone functions called with .then().