restify/clients

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:

  1. If the client fails to parse the payload it should return a sensible parse error with useful information
  2. If there is an HTTP error, that error trumps the parse error. Alternatively, we could return a MultiError - open to discussion on that.
  3. 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.