Feature/improve xml request error handling

[liamaharon]

Description

Adds explicit handling for XMLHttpRequest errors.

When an XMLHttpRequest fails, instead of throwing Error: Invalid JSON RPC response: "" throw Error: HTTP request failed.

Unfortunately, accessing the specifics about what caused a request error when using XMLHttpRequest is not trivial.

In future versions of web3.js maybe XMLHttpRequest could be substituted for fetch, which is built into modern browsers (so no increase to bundle size). As well making informative error handling really simple, a Fetch implementation would require far fewer lines of code and less complexity in the packages/web3-providers-http/src/index.js file.

Fixes #3425

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation. Unsure where this would need to be documented?
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run dtslint with success and extended the tests and types if necessary.
• I ran npm run test:unit with success.
• I have executed npm run test:cov and my test cases do cover all lines and branches of the added code. Unsure if required to write tests for this change as there don't appear to be tests for similar logic branches, happy to if it's required just let me know
• I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
• I have updated the CHANGELOG.md file in the root folder. Did I do this right?
• I have tested my code on the live network.

First time opening a PR here so let me know if I made any mistakes, happy to revise ✌️

[coveralls]

Coverage decreased (-0.05%) to 90.114% when pulling 8914868 on liamaharon:feature/improve-xml-request-error-handling into 70158a8 on ethereum:1.x.

[nivida]

It would probably be great to get our users some more information about the failed requests. :-)

[liamaharon]

Agreed :)

I wrote a little bit about this towards the end of the PR description, would you be open to having XMLHttpRequest being replaced with another library like fetch as part of this PR?

[wbt]

Would it be too much to ask to request an error number be included, instead of just an error string, to avoid upgrade issues like this with future string clarifications?

[GregTheGreek]

This would be nice to revive, @liamaharon do you mind updating your branch accordingly and we can take a look at it :)

[liamaharon]

@wbt I think this is sensible. The easiest (and laziest) way to implement this would be to add a prefix or suffix to all the error strings, for example something like Error ({errCode}): {errMsg} or {errMsg} (code {errCode}). This way you could match for the error code, rather than the error message in the string. Is this along the lines of what you're thinking?

@GregTheGreek sure, I've merged the latest 1.x into my branch. Let me know if anything else needs doing, happy to work with you to move this forward.

[wbt]

@liamaharon

@wbt I think this is sensible. The easiest (and laziest) way to implement this would be to add a prefix or suffix to all the error strings, for example something like Error ({errCode}): {errMsg} or {errMsg} (code {errCode}). This way you could match for the error code, rather than the error message in the string. Is this along the lines of what you're thinking?

That still seems a bit hacky. In more professionalized packages, the errors thrown are objects rather than strings, with members "message" (the existing error message), "code" (a numeric code), and sometimes other fields. I was thinking along those lines.

[github-actions]

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[uluhonolulu]

Please don't close it, the current error message is too confusing and requires very specific know-how to understand.

[github-actions]

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[wbt]

@uluhonolulu LOL; bots apparently don't read.

[uluhonolulu]

@GregTheGreek I would like to pick up this PR as @liamaharon asked me. The only problem is I've never worked on somebody else's PR's, so I don't know how to do it technically. Should I open a new PR or contribute to this one somehow? If the latter, can you assign it to me, please?

[GregTheGreek]

@GregTheGreek I would like to pick up this PR as @liamaharon asked me. The only problem is I've never worked on somebody else's PR's, so I don't know how to do it technically. Should I open a new PR or contribute to this one somehow? If the latter, can you assign it to me, please?

I would branch off of this pr and open a new one!

[uluhonolulu]

OK, I'm on it

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[github-actions]

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[jdevcs]

Closing this PR in favor of #3425 (comment)