Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

JSON.parse error reporting: with invalid json: in the error string #1001

Closed
wants to merge 6 commits into from
Closed

Conversation

KrishnaPG
Copy link
Contributor

@KrishnaPG KrishnaPG commented May 16, 2019

Better error reporting in case of json.parse failures

resolves #912
resolves #1000

Better error reporting in case of `json.parse` failures
@KrishnaPG KrishnaPG changed the title Update stream-to-json-value.js (With invalid json: data in the error JSON.parse error reporting: with invalid json: in the error string May 16, 2019
@hugomrdias
Copy link
Contributor

@alanshaw this is only failing on commitlint should be good to merge

@alanshaw
Copy link
Contributor

I think @KrishnaPG is going to update this to be even better ;)

#1000 (comment)

Making the `parseError` function compatible with non-JSON responses from server.
   - `isJson = true` parameter added that is backwards compatible with existing code

Ref: #1000 (comment)
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there - just some minor tweaks and this'll be good. Would love to see some tests for this!?

KrishnaPG and others added 3 commits May 17, 2019 04:49
`parseError` method should print default error in case data is null or empty ("")

Co-Authored-By: Alan Shaw <alan.shaw@protocol.ai>
This makes `parseError` more self-sufficient.
  35:1   error    Trailing spaces not allowed   no-trailing-spaces
This is needed for the callers to know when the error needs a retry (such as 401, 403 etc.
@KrishnaPG
Copy link
Contributor Author

The remaining lint error is about handling the error in callback

/home/travis/build/ipfs/js-ipfs-http-client/src/utils/send-request.js
   25:31  error    Expected error to be handled  handle-callback-err

By design we are ignoring that error, since we are already inside an error handler (with a valid error response from server).

Is there a way to turn this lint error off, for this specific case, so that this can be merged?

@KrishnaPG
Copy link
Contributor Author

KrishnaPG commented May 21, 2019

The changes in this PR now enables usage such as:

ipfs.block.put(Buffer.from("Hello " + Date()), (err, block) => {
  if (err) {
     if (err.code == 401 || err.code == 403) 
        return Retry_With_Auth(block); // retry this whole thing with renewed AUTH Headers somehow
     // else some really problematic error
  }
  // success
})

Earlier this was not possible, since the textual response from gateways was crashing the app.

@alanshaw
Copy link
Contributor

@KrishnaPG I've rebased, added tests and changed code to statusCode (since it conflicts with the code in the JSON response) here: #1016

alanshaw pushed a commit that referenced this pull request May 21, 2019
Better error reporting by detecting `Content-Type` of the response and not attempting to parse JSON for non-`application/json` responses.

resolves #912
resolves #1000
closes #1001

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON.parse(data) errors Better handling for unexpected responses
3 participants