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!?

src/utils/send-request.js Outdated Show resolved Hide resolved
src/utils/send-request.js Outdated Show resolved Hide resolved
src/utils/send-request.js Outdated Show resolved Hide resolved
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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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