RelayDefaultNetworkLayer, throw Error also on non-200 response. #1163
Conversation
ghost
added
the
| @@ -2,7 +2,7 @@ | |||
| parser: babel-eslint | |||
|
|
|||
| extends: | |||
| - ./node_modules/fbjs-scripts/eslint/.eslintrc | |||
| - ./node_modules/fbjs-scripts/eslint/.eslintrc.js | |||
There was a problem hiding this comment.
This is going to conflict with #1161
Can you please revert this bit or rebase and then we can import this.
|
Thanks @Globegitter. This looks pretty good to me. One comment inline though, otherwise looks good to import. |
|
@wincent Oh yeah sorry forgot to remove that again. Just rebased. |
|
@facebook-github-bot import |
|
Thanks for importing. If you are an FB employee go to Phabricator to review. |
| networkLayer.sendMutation(request); | ||
| expect(fetch).toBeCalled(); | ||
| const failureResponse = genFailureResponse(response); | ||
| expect(failureResponse.status >= 300 || failureResponse.status < 200).toBeTruthy(); |
There was a problem hiding this comment.
Since failureResponse isn't part of the network layer, let's not make assertions about it in the test body.
|
Ping! I left some comments in the code that came up after an internal code review. |
|
@steveluscher sorry about the delay, was a bit busy and then went on holiday (well still am). Just looked over sharing the code and yes we can share some code, but they are also slightly different scenarios (i.e. a non-200 response is might to not return json). Overall I do think I agree with you though so I'll updating the PR now. |
|
Only typecheck (same as on master) is failing so good to review again @steveluscher |
| @@ -132,7 +122,7 @@ class RelayDefaultNetworkLayer { | |||
| method: 'POST', | |||
| }; | |||
| } | |||
| return fetch(this._uri, init).then(throwOnServerError); | |||
| return fetch(this._uri, init).then(throwOnServerError.bind(this, request)); | |||
There was a problem hiding this comment.
Why .bind(…)? I couldn't see a reference to this inside that function body.
There was a problem hiding this comment.
OH. The request argument. I see.
There was a problem hiding this comment.
Yeah, it wasn't a 100% ideal but that seemed to me the best way to share the error creation in all three places (or of course an arrow function if that has any advantages).
Right now the mutation transacion gets its error attribute set differently on server-side non-200 responses (e.g. 500) compared to 200 responses that include the
errorsattribute in the json. On the latter it is anErrorobject as described in the docs but on the first we still get the normal fetchResponse. (So https://facebook.github.io/relay/docs/api-reference-relay-store.html#commitupdate-static-method is not completely right)That also means handling these two different cases in the mutation
onFailurecall requires two very different pieces of logic.This PR now makes it so that transaction.getError() always returns an
Error, and if it was a non-200 response it also includes that status code.