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

When Non-OK Response Received from fetch, throw an error including the response #1205

Merged
merged 10 commits into from Jan 25, 2017

Conversation

@pseudoramble
Copy link
Contributor

pseudoramble commented Jan 20, 2017

This is intended to resolve Issue 1199 and Issue 542.

This provides the Response provided from fetch when a non-OK status has been received from the server. It includes a new error (HttpNetworkError) so the response & request could be given to the caller. This change also includes a small testing function to create a fake IResponse instances.

I noticed that these issues are around a similar situation, but have somewhat different designs. If this change is too large for this PR, I'll be glad to break it up (one for each issue).

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • Add your name and email to the AUTHORS file (optional)
…K response is received from the server.

It includes a new error called HttpNetworkError which includes the response which failed.
A new fake IResponse interface function was created to help aid in testing.
@apollo-cla

This comment has been minimized.

Copy link

apollo-cla commented Jan 20, 2017

@pseudoramble: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@pseudoramble pseudoramble changed the title Fix: When Non-OK Response Received from fetch, throw an error including the response When Non-OK Response Received from fetch, throw an error including the response Jan 20, 2017
@@ -0,0 +1,20 @@
export default class HttpNetworkError extends Error {

This comment has been minimized.

Copy link
@calebmer

calebmer Jan 20, 2017

Contributor

How does this integrate with ApolloError? Could we just add request and response to ApolloError network errors? Should we change the type of networkError here to HttpNetworkError?

I’d like to see a better story around HTTP network errors like this one, and the errors already handled by Apollo Client. I’m also kind of uncomfortable adding a new error class without a clear reason why.

This comment has been minimized.

Copy link
@calebmer

calebmer Jan 20, 2017

Contributor

Also, does this PR make all HTTP network errors HttpNetworkError? If not then we might need a new name as well.

This comment has been minimized.

Copy link
@pseudoramble

pseudoramble Jan 21, 2017

Author Contributor

This will be the value of the networkError provided on ApolloError instances. Callers to the networkInterface take its errors and replace them as networkError. Since I wasn't positive that all networkErrors could actually be specifically related to HTTP issues, I did not want to make the error more specific. It does seem like a concrete type for network-related errors would be helpful. Maybe NetworkError would be a more suitable name though.

I can take a look at providing the response on a plain Error instance. Is it OK to attach the response like this?

const networkError = new Error(`Network request failed with status ${resp.status} ${resp.statusText}`);
networkError.response = resp;
throw networkError;

Or is there another way to approach this?

This comment has been minimized.

Copy link
@calebmer

calebmer Jan 23, 2017

Contributor

How about we attach the response to any error thrown in our network interfaces. So in the following locations here: https://github.com/pseudoramble/apollo-client/blob/e4a325ab5a7471706759b723b0fed58c3c72829d/src/transport/batchedNetworkInterface.ts#L105 and https://github.com/pseudoramble/apollo-client/blob/e4a325ab5a7471706759b723b0fed58c3c72829d/src/transport/batchedNetworkInterface.ts#L109

…we would change:

reject(error)

…to:

error.response = response
reject(error)

…and for the promise chain here: https://github.com/pseudoramble/apollo-client/blob/e4a325ab5a7471706759b723b0fed58c3c72829d/src/transport/networkInterface.ts#L178-L206

…we would add a catch which looks like:

promise.catch(error => {
  error.response = response
  throw error
})

…that just mutates the error and re-throws it for the next promise handler.

Not only would this change expose the same information as a new error subclass, but it would also allow users to get the response on any network error that may be thrown. This could have some nice unforeseen benefits 👍

I’m not worried about type safety for errors because most asynchronous types in TypeScript give errors the type of any. This includes the standard types for Promise and some of the more popular types for Observable.

This comment has been minimized.

Copy link
@pseudoramble

pseudoramble Jan 24, 2017

Author Contributor

Cool. Makes sense to try and catch any sort of general errors we may see here and skip on a new error at this point. I'll work on removing the new error tonight.

I'm confused about how I would access the response in these more specific catch's though. I don't believe I would have access to the response once the promise was rejected. I could attempt to save the response temporarily and attach it later like this:

let savedResponse = null;
return this.applyMiddlewares(...)
...then(({ response }) => {
  savedResponse = response;
})
...catch(err => {
  (err as any).response = savedResponse;
  throw err;
});

Was this what you were thinking of, or am I missing something? I'll keep investigating to try and get what you're looking for, but I'm not quite seeing it yet.

const httpResponse = result as IResponse;

if (!httpResponse.ok) {
throw new HttpNetworkError({

This comment has been minimized.

Copy link
@calebmer

calebmer Jan 20, 2017

Contributor

Could we make a more descriptive error message here? Like: Network request failed with status 404 "not found".

This comment has been minimized.

Copy link
@pseudoramble

pseudoramble Jan 22, 2017

Author Contributor

Done. HttpNetworkError generates this message by default. If we remove HttpNetworkError I'll move it back onto each place where it's thrown.

status: number,
statusText?: string,
body?: string,
): IResponse {

This comment has been minimized.

Copy link
@calebmer

calebmer Jan 20, 2017

Contributor

This is incredibly thorough. Any reason why we can’t return a MockedIResponse? Or return a MockedIResponse as any if TypeScript complains?

This comment has been minimized.

Copy link
@pseudoramble

pseudoramble Jan 21, 2017

Author Contributor

It was mainly to help support testing by providing an instance of IResponse which the HttpNetworkError added looks for. I'll look into removing this and replacing it with MockedIResponse if I can.


const err = new HttpNetworkError({ response });

assert.deepEqual(err.response, response);

This comment has been minimized.

Copy link
@calebmer

calebmer Jan 20, 2017

Contributor

Could we make this strict equal and not deep-equal?

@pseudoramble

This comment has been minimized.

Copy link
Contributor Author

pseudoramble commented Jan 21, 2017

@calebmer - Thanks for the review! I appreciate the time and effort.

You mentioned wanting a better story around why this change would be needed. I'll try and elaborate here a bit. If this isn't what you were looking for, my apologies.

When you deal with a network that may have issues (a backing service goes down or the user is required to be authenticated) it's hard to determine what the network-related issue was. So when doing this today:

try {
  const { data } = await graphql.query({ query });
} catch (err) {
  console.log("network error = ", err.networkError.message);
}

You will get a networkError, but the error message will look something like this because of Issue 1199

Uncaught SyntaxError: Unexpected token U in JSON at position 0

That's because the response is assumed to be OK at this point and will attempted to be parsed. Depending on the body of the message, the error may look different.

This can be fixed by making the adjustments outlined in Issue 1199. Without having the response provided though, networkError.message would need to be interpreted by the caller, which could be error-prone. Giving a specific error that provides the response and request data that failed seems helpful.

try {
  const { data } = await graphql.query({ query });
} catch (err) {
  console.log("HTTP Error Status:", err.networkError.response.status);
  console.log("HTTP Error Message:", err.networkError.response.statusText);
}

If adding another error is something we want to avoid, I'll take a look into avoiding that.

Thanks again!

pseudoramble and others added 3 commits Jan 21, 2017
… test with `strictEqual`.
This eliminates the testing code needed for IResponse.
Updated the HttpNetworkError to accept an interface that provides a status plus optional statusText.
@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Jan 23, 2017

Yep, I totally agree that we need to throw a better error and fix #1199. I’m just not convinced another error class is the solution so let’s definitely look into ways to attach the response without a new class. I think I have an idea, I’ll leave another comment in the code 👍

Copy link
Contributor

calebmer left a comment

Looks good. If you could just make the change mentioned below so we can avoid another error subclass that would be excellent 😊

@@ -0,0 +1,20 @@
export default class HttpNetworkError extends Error {

This comment has been minimized.

Copy link
@calebmer

calebmer Jan 23, 2017

Contributor

How about we attach the response to any error thrown in our network interfaces. So in the following locations here: https://github.com/pseudoramble/apollo-client/blob/e4a325ab5a7471706759b723b0fed58c3c72829d/src/transport/batchedNetworkInterface.ts#L105 and https://github.com/pseudoramble/apollo-client/blob/e4a325ab5a7471706759b723b0fed58c3c72829d/src/transport/batchedNetworkInterface.ts#L109

…we would change:

reject(error)

…to:

error.response = response
reject(error)

…and for the promise chain here: https://github.com/pseudoramble/apollo-client/blob/e4a325ab5a7471706759b723b0fed58c3c72829d/src/transport/networkInterface.ts#L178-L206

…we would add a catch which looks like:

promise.catch(error => {
  error.response = response
  throw error
})

…that just mutates the error and re-throws it for the next promise handler.

Not only would this change expose the same information as a new error subclass, but it would also allow users to get the response on any network error that may be thrown. This could have some nice unforeseen benefits 👍

I’m not worried about type safety for errors because most asynchronous types in TypeScript give errors the type of any. This includes the standard types for Promise and some of the more popular types for Observable.

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Jan 23, 2017

@calebmer feel free to merge this once you think it's good to merge!

…ature/add-network-error
This attaches the request and response to the typical error object. It also moves the message generated back into the networkInterface/batchedNetworkInterface.
if (!httpResponse.ok) {
const httpError = new Error(`Network request failed with status ${httpResponse.status} - "${httpResponse.statusText}"`);
(httpError as any).response = httpResponse;
(httpError as any).request = requestsAndOptions;

This comment has been minimized.

Copy link
@calebmer

calebmer Jan 24, 2017

Contributor

Do we really need to add the request here? Especially given that here request is actually an array of requests, but in the normal network interface it is a single object.

I’d like to just remove request entirely for now from the error unless there’s an argument to why we really need it. Since this ends up getting thrown at the query site the request information may not be necessary. After that, this looks good to merge 👍

This comment has been minimized.

Copy link
@pseudoramble

pseudoramble Jan 25, 2017

Author Contributor

No specific reason to include the requests other than it was something of interest in one of the issues. I'll make this modification tonight!

Thanks again for all your help on this!

This comment has been minimized.

Copy link
@pseudoramble

pseudoramble Jan 25, 2017

Author Contributor

This is finished up now.

@calebmer calebmer merged commit 296b2be into apollographql:master Jan 25, 2017
3 checks passed
3 checks passed
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 96.348%
Details
@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Jan 25, 2017

Looks good, this will be included with the next release 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.