Fix/improve error handling

DickSmith commented 6 years ago

Included latest typings from net.gotev.uploadservice which shows a change in the signature of onError from: onError(context: any, uploadInfo: UploadInfo, error) {} to: onError(context: Context, uploadInfo: UploadInfo, serverResponse: ServerResponse, error: java.lang.Exception) {} Also, the error was also not being passed to the new zonedOnError function.

Also added the responseCode in the ErrorEventData in a cross-platform matter to allow apps to respond accordingly.

PR Checklist

[x] The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
[ ] There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
[N/A (no tests in this repo)] All existing tests are passing
[N/A (no tests in this repo)] Tests for the changes are included

What is the current behavior?

ServerResponse is being passed as the error rather than the java.lang.Exception in the current method, but then isn't actually being passed to zonedOnError, either. The response code is not available.

What is the new behavior?

The Exception is now being passed correctly as the error. The responseCode is now available for both iOS and Android, or -1 if the response is invalid.

BREAKING CHANGES: None, since currently the error isn't being passed up at all on Android, and the responseCode is a new property for both.

DickSmith commented 6 years ago

@VladimirAmiorkov I added the responseCode to Result per your request. I also switched background-http.ios.ts to use spaces rather than tabs to make it consistent with the other files.

DickSmith commented 6 years ago

Ah, I just reread your comment.

What I was thinking of doing, Android is already returning the response object there, but iOS is not, do you still want the response returned there? or just the responseCode?

DickSmith commented 6 years ago

OK, so I added the responseCode to complete as well. I removed the response that only Android was putting there, I could add the "response" to the iOS data as well, but these will be platform specific and the responseCode should suffice better, so potentially a breaking change on Android, but sending the full response object in Android was probably too heavy and unused anyway.

Let me know your thoughts.

VladimirAmiorkov commented 6 years ago

Hi @DickSmith ,

I see what you mean about the response inside the complete event on Android and I agree that it could be redundant if you only want to retrieve the response code from it. But that ServerResponse contains additional functionality like getHeaders() etc. that brings value to the event arguments. Just for reference the same can be achieve on iOS using the object like so args.object.ios.response.allHeaderFields so removing the response entirely from the event in Android should not be done.

I would suggest to either expose the native response on iOS also which is currently a missing functionality (two birds with one stone) or simply return the response inside the complete event on Android and simply add the new responseCode so I can proceed with merging this PR. I understand that providing platform specific objects in event callbacks is not great but sometimes it is better to provide the entire native object than adding multiple properties one by one to the arguments with each new version of the plugin. We will be happy if you could help us with achieving full platform unity in this API but I suggest you to do it in a separate PR to not make this one too big and keep it relevant to the responseCode exposure.

We appreciate your work on this.

DickSmith commented 6 years ago

@VladimirAmiorkov Ah, for sure, I could've go either way on it. If we want the response to actually be exposed then we should add it to the CompleteEventData interface so that it is clearer that it is intended. That was the reason I defaulted to removing it in this case, since it wasn't declared in any of the typings and wasn't on the iOS, so I was unsure if it was intended or not, or just an artifact of previous versions of the plugin or for debug during development that got left behind.

I'll add a JSDoc to the response object and detail that it is currently "android-only" for now.

Would be nice to unify those for sure, but yeah, I don't currently have the time to plow deeper on this one myself, and the responseCodes satisfy all my current needs.

DickSmith commented 6 years ago

@VladimirAmiorkov Done, let me know if that works.

VladimirAmiorkov commented 6 years ago

@DickSmith This is available with version 3.2.5

NativeScript / nativescript-background-http