Skip to content
This repository has been archived by the owner on Sep 26, 2022. It is now read-only.

iOS: request error does not reject plugin call #224

Open
mrahn24 opened this issue Jan 27, 2022 · 9 comments · May be fixed by #225
Open

iOS: request error does not reject plugin call #224

mrahn24 opened this issue Jan 27, 2022 · 9 comments · May be fixed by #225

Comments

@mrahn24
Copy link

mrahn24 commented Jan 27, 2022

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 added a commit to mrahn24/http that referenced this issue Jan 27, 2022
@mrahn24 mrahn24 linked a pull request Jan 27, 2022 that will close this issue
mrahn24 added a commit to mrahn24/http that referenced this issue Jan 27, 2022
@funkyvisions
Copy link

funkyvisions commented Feb 8, 2022

Hmmmm. I reported #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
Copy link

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
Copy link

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.

@nickredding
Copy link

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 #232. That is actually a defect in Capacitor.fromNative

@mrahn24
Copy link
Author

mrahn24 commented Feb 22, 2022

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.

  • CAPLog.print("Error on upload file", String(describing: data), String(describing: response), String(describing: error))
    call.reject("Error", "UPLOAD", error, [:])
  • CAPLog.print("Error on download file", String(describing: downloadLocation), String(describing: response), String(describing: error))
    call.reject("Error", "DOWNLOAD", error, [:])

@nickredding
Copy link

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

@neonish
Copy link

neonish commented Feb 27, 2022

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.

@funkyvisions
Copy link

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.

mrahn24 added a commit to mrahn24/http that referenced this issue Feb 28, 2022
* fix: Fixed missing request plugin call rejection
* refactor: Adjusted error messages to provide better information
@mrahn24
Copy link
Author

mrahn24 commented Feb 28, 2022

@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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants