Reversing API status code #962
Conversation
|
Just for a bit more context: factom.js is breaking indirectly because the http client it uses, axios, handle non 2xx response as rejected Promises, which goes to a different path ( |
|
ok, this is being tracked with ticket FD-1281 |
carryforward
changed the base branch from
master
to
FD-1281_revert_http_error_codes
Correct me if I'm wrong but it seems like factom.js is misusing the |
|
I would love to see this get merged back, and factom.js be fixed. The FAT/factom package, and fatd that uses it, depends on distinguishing between a network error and a bad lookup. In some cases both errors are treated as fatal, but when it comes to checking to see if an identity is yet populated, the distinction becomes important. |
It seems at this point, we just don't know how many projects rely on factom.js and/or the api status code, so changing it for V2 is unlikely as it would break too many existing projects. This is something now slated for a V3 api, I believe. |
|
Axios (which for context is one of the most popular HTTP client with 7M weekly downloads) has made the design choice to "route" separately 2xx HTTP response from others. Axios is not aware of RPC, it's just a HTTP client. So as a client I have to use their interface and because factomd returns 4xx response for an error, I have no choice but to handle that in the catch, not up to me to decide. Which is my core point, factomd made a mistake in their RPC2 implementation and clients had no choice but to adapt to this mistake. Of course I totally wish it'd be fixed, but that's just not possible in a backward compatible way. It shows up as a problem in factom.js but could break any other client that worked around factomd current peculiarities... I have no problem tweaking factom.js to handle both cases and publish a new version, but that would still not make me approve this change implemented this way because of the unknown/unbounded risk of breakage across the entire ecosystem. The correct way to handle this non-backward compatible change is to bump the API version. |
|
Thanks for the clarification. Makes good sense.
Perhaps it would make sense to start an experimental v3 api that is allowed
to make these sorts of changes and is advertised as not stable.
…On Thu, Feb 13, 2020, 8:35 AM Paul Bernier ***@***.***> wrote:
Axios (which for context is one of the most popular HTTP client with 7M
weekly downloads <https://www.npmjs.com/package/axios>) has made the
design choice to "route" separately 2xx HTTP response from others. Axios is
not aware of RPC, it's just a HTTP client. So as a client I have to use
their interface and because factomd returns 4xx response for an error, I
have no choice but to handle that in the catch, not up to me to decide.
Which is my core point, factomd made a mistake in their RPC2 implementation
and clients had no choice but to adapt to this mistake. Of course I totally
wish it'd be fixed, but that's just not possible in a backward compatible
way. It shows up as a problem in factom.js but could break any other client
that worked around factomd current peculiarities...
I have no problem tweaking factom.js to handle both cases and publish a
new version, but that would still not make me approve this change
implemented this way because of the unknown/unbounded risk of breakage
across the entire ecosystem. The correct way to handle this non-backward
compatible change is to bump the API version.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#962?email_source=notifications&email_token=ABOZO4AUPODZ7KS3PQ3AKY3RCWAE7A5CNFSM4KPROMI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELV4Z7I#issuecomment-585878781>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABOZO4ASJSK5NKR5VXD4VNDRCWAE7ANCNFSM4KPROMIQ>
.
|
|
I fixed my jsonrpc2 so that it is not so rigid in enforcing http status codes and always tries to unmarshal the response body. So this won't be an issue for my factom library soon. |
This is a compatibility issue that came to light in testing 6.5.1. PR #897 changed the API to return status code 200 for json-rpc errors, but the factom.js client relies on status codes to determine whether or not a request is an error. Functionality is being changed back to preserve backward compatibility.
Reversing this means the FAT/factom client is no longer fully compatible due to AdamSLevy/jsonrpc2#3. However, this is a fairly benign issue that results in FAT/factom to return a json-parsing error rather than an appropriate error.