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.

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

On an XMLHttpRequest error, expose error message #565

Closed
aarondail opened this issue Sep 22, 2017 · 6 comments
Closed

On an XMLHttpRequest error, expose error message #565

aarondail opened this issue Sep 22, 2017 · 6 comments

Comments

@aarondail
Copy link

I am trying to get a more useful error message out of fetch when there are network errors...

For context: I am using react native, which uses this library internally for its fetch implementation. If I am connecting to a server whose cert is invalid, or have a typo in the server name, its nicer to expose a more descriptive error message to the user. The XMLHttpRequest that react native provides does this, but fetch ignores it and always sends back 'Network request failed'.

I believe this is because: in the fetch constructor the onerror and ontimeout callbacks from the XMLHttpRequest object are wired up like this:

      xhr.onerror = function() {
        reject(new TypeError('Network request failed'))
      }

      xhr.ontimeout = function() {
        reject(new TypeError('Network request failed'))
      }

Sadly, no information about what the error was from the XMLHttpRequest object is actually exposed.

Could it be changed to something like this:

      xhr.onerror = function() {
        reject(new TypeError(xhr.responseText || 'Network request failed'))
      }

      xhr.ontimeout = function() {
        reject(new TypeError(xhr.responseText || 'Network request failed'))
      }
@mislav
Copy link
Contributor

mislav commented Sep 24, 2017

There's some value in what you're proposing, but see #562 (comment) for more thoughts on the subject.

Would xhr.responseText be reliable as a source of error message?

@aarondail
Copy link
Author

Sorry for the delay in responding :)

I dont know enough to answer your question. In the context of react native, it is. I honestly dont know about other contexts where fetch is used (browser, and others).

@dewwwald
Copy link

dewwwald commented Mar 26, 2018

I am a fullstack developer. I work on IOS, Web and NodeJs platforms, I might be of assistance here. This implementation would also be a tremendous benefit to me. I believe errors should not be recreated and concealed as they are in this library, or at least not in the way that it has been done here. (debugging is really difficult if I don't know what the cause of my error is)

I can do some digging and make a PR as soon as possible, I will be adding comments here. Would you guys mind checking my comments and telling me what you think while I work on this?

@dewwwald
Copy link

dewwwald commented Mar 26, 2018

Chrome implementation of error object for the fetch lib:
In the console of the browser you can read a message: Uncaught (in promise) TypeError: Failed to fetch, however before that is additional output. As follow in this screenshot:
screen shot 2018-03-26 at 17 17 41

Firefox implementation of error object for the fetch lib:
screen shot 2018-03-26 at 17 55 47

Safari implementation of error object for the fetch lib:
screen shot 2018-03-26 at 17 58 35

My point with these screengrabs is that there are things we can observe:

  • Error message is clear
  • Error message is not concealed behind a generic message
  • Warning regarding CORS are show to ensure full awareness of the engineer (even if a catch is present)

Since this is a polyfil we need to consider the XmlHttpResponse response objects limitations on all browsers when showing a message, even so I think we can do better.

Todo

This is just a list of items I think has to be done for this to be considered complete.

  • determine the prototypes of the xmlHttpResponse on all known major platforms
    • IOS (React Native)
    • Andriod (React Native)
    • ~~~Chrome~~~ Not relevant to the scope of this library
    • ~~~Firefox~~~ Not relevant to the scope of this library
    • Internet explorer 9
    • Internet explorer 10
    • Internet explorer 11
  • Edge ??
  • write an implementation that shows a warning for unusable responses like CORS errors
  • write an implementation that enriches the onerror function referenced below
  • write a test to check each browser if the implementation hill have the desired data points

Edit

I was looking in to this, there is not real clear data points to use. I can understand why there was not better implementation here.

@mislav
Copy link
Contributor

mislav commented Mar 29, 2018

@dewwwald Thanks for looking into this! I appreciate your notes.

@mislav
Copy link
Contributor

mislav commented Sep 3, 2018

Closing since there is nothing left actionable for us to do after this discussion.

If anyone has ideas how to improve these error messages, PRs welcome. But please consider all the points above.

@mislav mislav closed this as completed Sep 3, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants