Handling JSON parsing errors in JSONClient
DonutEspresso opened this issue · 2 comments
I have a proposal for better JSONClient ergonomics. Currently, the client swallows potential JSON.parse errors, then creates an empty object and sets that as the parsed result:
https://github.com/restify/clients/blob/master/lib/JsonClient.js#L65
This is confusing and can be very misleading. JSONClient should do best effort parsing on incoming payloads. Seeing that the JSON RFC has been expanded to include strings as valid JSON, i.e., '"hello"'
, I propose the following:
- If the client fails to parse the payload it should return a sensible parse error with useful information
- If there is an HTTP error, that error trumps the parse error. Alternatively, we could return a MultiError - open to discussion on that.
- If the client fails to parse the payload, it should return the original string payload instead of an empty object
These proposals would address #138 and #140 for more comprehensive JSON handling.
This breaks:
- restify/node-restify#729 - would argue that JSONP makes more sense in a separate JSONP client, given that JSONP is a completely different content type (
application/javascript
) - restify/node-restify#203 - loose behavior that we are trying to close/address with this PR
Thoughts? cc @yunong @rajatkumar @retrohacker @hekike
Just for the record, based on the PR we don't do item 3.
However, I think it would be a good idea to add the original string payload to the error.
Good call, missed a commit locally, just updated PR.