refactor: simplify FetchrError and fix Error inheritance #352
Conversation
It's possible to remove it since native fetch always return a string if we have a response and since we only have an error when we have no response.
It's possible to do that since we will always have a bad status code.
It was necessary to create a error class in the server since there are tests shared test code between server and client that can only work with one class of error.
libs/util/httpRequest.js
Outdated
| response, | ||
| responseBody | ||
| ); | ||
| return response.text().then(function (body) { |
There was a problem hiding this comment.
is body an error here? should we call it error instead?
There was a problem hiding this comment.
Do you mean an Error instance or that the body contains an error message? Well, it's a string and it might contain some error message yes.
But I think it makes sense what you said, however, I think renaming to err could be misleading since, normally, err points to an Error instance. It gets more awkward since just a few lines below, we have another err name, and that one is a true error.
So I think renaming to message could be a good option (since this is the name of FetchrError param as well). What do you think?
There was a problem hiding this comment.
message makes sense to me
| } | ||
|
|
||
| FetchrError.prototype = Object.create(Error.prototype); |
There was a problem hiding this comment.
Why not make it a proper class?
There was a problem hiding this comment.
To keep es5 compatibility. I will transform it as soon as we move to TS, but for now, I think it's better to keep es5 syntax compatibility.
pablopalacios
force-pushed
the
refactor-error
branch
from
e38a476 to
047536d
Compare
After migrating from xhr to native fetch, many cases inside
FetchrErrorwere not reachable anymore. I didn't refactor it back that time since the code was super hard to understand and we didn't have functional tests to see the real consequences of updating it. But now it was a breeze.Hopefully, FetchrError class is easier to understand now. I also briefly document it in the README and, as you can see, the data inside the error could be better and simplified. But it would introduce some breaking changes that I would postpone for V1 now.
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.