-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Changes from 5 commits
3deb7e5
4e81f4c
e1ce16d
3826c93
e4a325a
33bae9f
df13561
c895055
a1d9009
b5ae6f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
export interface Response { | ||
status: number; | ||
statusText?: string; | ||
} | ||
|
||
export default class HttpNetworkError extends Error { | ||
public readonly response: Response; | ||
public readonly request: any; | ||
public readonly message: string; | ||
|
||
constructor({ | ||
response, | ||
request = {}, | ||
message, | ||
}: { | ||
response: Response, | ||
request?: any, | ||
message?: string, | ||
}) { | ||
const defaultMessage = `Network request failed with status ${response.status} - "${response.statusText}"`; | ||
super(message || defaultMessage); | ||
|
||
this.response = response; | ||
this.request = request; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ import { | |
QueryBatcher, | ||
} from './batching'; | ||
|
||
import HttpNetworkError from '../errors/HttpNetworkError'; | ||
|
||
import { assign } from '../util/assign'; | ||
|
||
// An implementation of the network interface that operates over HTTP and batches | ||
|
@@ -64,12 +66,19 @@ export class HTTPBatchedNetworkInterface extends HTTPFetchNetworkInterface { | |
Promise.all(middlewarePromises).then((requestsAndOptions: RequestAndOptions[]) => { | ||
return this.batchedFetchFromRemoteEndpoint(requestsAndOptions) | ||
.then(result => { | ||
const httpResponse = result as IResponse; | ||
|
||
if (!httpResponse.ok) { | ||
throw new HttpNetworkError({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we make a more descriptive error message here? Like: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
request: requestsAndOptions, | ||
response: httpResponse, | ||
}); | ||
} | ||
|
||
// XXX can we be stricter with the type here? | ||
return result.json() as any; | ||
}) | ||
.then(responses => { | ||
|
||
|
||
if (typeof responses.map !== 'function') { | ||
throw new Error('BatchingNetworkInterface: server response is not an array'); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
import { assert } from 'chai'; | ||
import { ApolloError } from '../src/errors/ApolloError'; | ||
import HttpNetworkError from '../src/errors/HttpNetworkError'; | ||
|
||
import { createMockedIResponse } from './mocks/mockFetch'; | ||
|
||
describe('ApolloError', () => { | ||
it('should construct itself correctly', () => { | ||
|
@@ -79,3 +82,31 @@ describe('ApolloError', () => { | |
assert(apolloError.stack, 'Does not contain a stack trace.'); | ||
}); | ||
}); | ||
|
||
describe('HttpNetworkError', () => { | ||
it('should provide the given response back to the user', () => { | ||
const response = createMockedIResponse({}, { status: 401, statusText: 'Unauthorized' }); | ||
|
||
const err = new HttpNetworkError({ response }); | ||
|
||
assert.deepEqual(err.response, response); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we make this strict equal and not deep-equal? |
||
}); | ||
|
||
it('should provide default values for the request and message', () => { | ||
const response = createMockedIResponse({}, { status: 403, statusText: 'Forbidden' }); | ||
const err = new HttpNetworkError({ response }); | ||
|
||
assert.isOk(err.message); | ||
assert.isObject(err.request); | ||
}); | ||
|
||
it('should accept a request and message if provided', () => { | ||
const response = createMockedIResponse({}, { status: 403, statusText: 'Forbidden' }); | ||
const request = { name: 'Sample Request' }; | ||
const message = 'a test message'; | ||
const err = new HttpNetworkError({ response, request, message }); | ||
|
||
assert.equal(err.message, message); | ||
assert.deepEqual(err.request, request); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this integrate with
ApolloError
? Could we just addrequest
andresponse
toApolloError
network errors? Should we change the type ofnetworkError
here toHttpNetworkError
?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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does this PR make all HTTP network errors
HttpNetworkError
? If not then we might need a new name as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be the value of the
networkError
provided onApolloError
instances. Callers to thenetworkInterface
take its errors and replace them asnetworkError
. Since I wasn't positive that allnetworkErrors
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. MaybeNetworkError
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?Or is there another way to approach this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
…to:
…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:…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 forPromise
and some of the more popular types forObservable
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 specificcatch
'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: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.