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

RelayDefaultNetworkLayer, throw Error also on non-200 response. #1163

Closed
6 changes: 5 additions & 1 deletion src/network-layer/default/RelayDefaultNetworkLayer.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,11 @@ function throwOnServerError(response: any): any {
if (response.status >= 200 && response.status < 300) {
return response;
} else {
throw response;
return response.text().then(text => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you feel about sharing the code that already handles mutation errors (see if (payload.hasOwnProperty('errors') of sendMutation())? Perhaps we should trigger the same error handling code when errors is present in the payload or when the status is non-200.

const error = new Error(text);
(error: any).status = response.status;
throw error;
});
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ describe('RelayDefaultNetworkLayer', () => {
};
}

function genFailureResponse(data) {
return {
json: () => Promise.resolve(data),
text: () => Promise.resolve(JSON.stringify(data)),
status: 500,
};
}

beforeEach(() => {
jest.resetModuleRegistry();

Expand Down Expand Up @@ -229,6 +237,31 @@ describe('RelayDefaultNetworkLayer', () => {
expect(error.source).toEqual(response);
});

it('handles server-side non-2xx errors', () => {
const response = {
errors: [{
message: 'Something went completely wrong.'
}],
};

expect(fetch).not.toBeCalled();
networkLayer.sendMutation(request);
expect(fetch).toBeCalled();
const failureResponse = genFailureResponse(response);
expect(failureResponse.status >= 300 || failureResponse.status < 200).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since failureResponse isn't part of the network layer, let's not make assertions about it in the test body.


fetch.mock.deferreds[0].resolve(failureResponse);
jest.runAllTimers();

expect(rejectCallback.mock.calls.length).toBe(1);
const error = rejectCallback.mock.calls[0][0];
expect(error instanceof Error).toBe(true);
expect(error.message).toEqual('{"errors":[{"message":"Something went ' +
'completely wrong."}]}');
expect(error.status).toEqual(failureResponse.status);
expect(error.source).toBe(undefined);
});

});

describe('sendQueries', () => {
Expand Down