http icon indicating copy to clipboard operation
http copied to clipboard

iOS: request error does not reject plugin call

Open mrahn24 opened this issue 4 years ago • 9 comments

Describe the bug When a request error occurs, e.g. the device has no internet connection, the plugin call does not getting rejected. As a result the error can not be handled by ionic.

To Reproduce Steps to reproduce the behavior:

  1. Implement a basic HTTP request with error handling, let's say a GET
  2. Turn off internet connection on device
  3. Send the request

=> The error does not get catched

Expected behavior The plugin call rejects and the error does get catched

Smartphone (please complete the following information):

  • Device: iPhone SE 2020
  • OS: iOS 15.2.1
  • Browser: Safari
  • Version:

Additional context

mrahn24 avatar Jan 27 '22 07:01 mrahn24

Hmmmm. I reported https://github.com/capacitor-community/http/issues/230. This would probably resolve the dns name failure (I hope)? Wonder if it would catch the timeout issue as well. I'm gonna try applying this and see.

funkyvisions avatar Feb 08 '22 21:02 funkyvisions

Hmmmm. I reported this #230. That would probably resolve the dns name failure (I hope)? Wonder if it would catch the timeout issue as well. I'm gonna try applying this and see.

Sweet! It looks like this handles the timeout issue as well. I'm gonna apply this patch locally.

funkyvisions avatar Feb 09 '22 14:02 funkyvisions

Even with this patch, the error object returned doesn't have much info. It looks like:

{"code":"REQUEST","errorMessage":"Error","line":3,"column":611,"sourceURL":"capacitor://localhost/js/chunk-vendors.fe43fbb8.js"}

When I do the logging in the swift file it looks like it has the "timeout" and "bad endpoint" errors, but they don't get propagated up. I wonder if this is true of all the errors being rejected in the swift code.

funkyvisions avatar Feb 09 '22 16:02 funkyvisions

mrahn24 - your fix does not seem to properly return the error information to the reject function. That function always receives the object

{ code: "REQUEST", error: "", errorMessage: "error", message: "error" }

which provides no information about the actual error.

Instead, the function request should call something like

call.reject(e.localizedDescription, "Http Request")

which returns the object (in this case for a timeout)

{ code: "Http Request", error: "", errorMessage: "The request timed out", message: "The request timed out" }

On an editorial note, I'm surprised that this plugin made it into the community pool without the most rudimentary checking of error handling. Another very basic defect exists with basic error handling as I noted in https://github.com/capacitor-community/http/issues/232. That is actually a defect in Capacitor.fromNative

nickredding avatar Feb 21 '22 23:02 nickredding

Hey there, thank you @funkyvisions @nickredding for your comments regarding the rejected error object.

I had implemented this differently before, but then decided to adapt to the behavior in the other functions, since I don't know the exact reason why the errors are rejected like this and like to have a consistent behavior, even if it's not the best. 😉 e.g.

  • https://github.com/capacitor-community/http/blob/3aca516560653349bd6f563b0d6c668e20827e03/ios/Plugin/HttpRequestHandler.swift#L238-L239
  • https://github.com/capacitor-community/http/blob/3aca516560653349bd6f563b0d6c668e20827e03/ios/Plugin/HttpRequestHandler.swift#L276-L277

mrahn24 avatar Feb 22 '22 07:02 mrahn24

mrahn24

I don't understand why you prefer

call.reject("Error", "REQUEST", error, [:])

over

call.reject(error.localizedDescription, "Http Request")

since the latter provides more error information than just "error". Note that the error object in your fix doesn't make it to the Javascript layer (error ends up as just an empty string) so there is no other way for the client code to get the error description.

If your pull request is to be merged I think you should update it to provide error.localizedDescription

nickredding avatar Feb 27 '22 00:02 nickredding

I am also experiencing the posted issue. I tested the changes of #225 locally and it does solve the error catching. Moreover, @nickredding's proposal gives sufficient information about the error:

{"code":"Http Request","errorMessage":"Could not connect to the server."}

I suggest using call.reject(error!.localizedDescription, "Http Request") and updating your PR.

neonish avatar Feb 27 '22 13:02 neonish

I’ve dumped this in favor of cordova-plugin-advanced-http by using awesome-cordova-plugins. It’s amazing how many of these Capacitor core plugins are broken or lacking functionality.

funkyvisions avatar Feb 27 '22 14:02 funkyvisions

@nickredding Thank you again for your comment!

I don't "prefer" the way the errors are currently handled, but I did it that way, because I don't know why the current error handlers were implemented as they are and what the maintainer's intention was by doing it that way.

I agree with you that the current error handling provides no real information to the JS layer, so I've updated the PR. Hope that this will solve your concerns as well.

Have a great day!

mrahn24 avatar Feb 28 '22 08:02 mrahn24