Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check for errors before trying to parse response #131

Merged
merged 2 commits into from
Mar 1, 2018
Merged

Conversation

tomwayson
Copy link
Member

resolves #110

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 14f5668 on fix/response-ok into 2a30d22 on master.

@Esri Esri deleted a comment from coveralls Feb 28, 2018
if (!response.ok) {
// server responded w/ an actual error (404, 500, etc)
const { status, statusText } = response;
return Promise.reject(new Error(`${status}: ${statusText}`));
Copy link
Contributor

Choose a reason for hiding this comment

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

@tomwayson should this be an ArcGISRequestError? Mainly so users can avoid situations like having to check for Error vs ArcGISRequestError:

request(/* ... */).catch(error => {
  if(error instanceof Error) {
    // ok I have an error so I only have error.message...
  }

  if(error instanceof ArcGISRequestError) {
    // ok I have an ArcGISRequestError so I can access lots of other stuff...
  }
})

It might be easier if users can do this:

request(/* ... */).catch(error => {
  // error is always a ArcGISRequestError
})

This could be something like:

return Promise.reject(new ArcGISRequestError(statusText, `HTTP ${status}`));

which woudl result in error messages like HTTP 404: Not Found

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give that a go later today

for consistency w/ rest of request and checkForErrors fns
@tomwayson tomwayson merged commit e2b05b2 into master Mar 1, 2018
@tomwayson tomwayson deleted the fix/response-ok branch March 1, 2018 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

check for error _before_ parsing response as JSON/text/whateves
4 participants