-
Notifications
You must be signed in to change notification settings - Fork 33
Conversation
Please test these changes in Expo after build has finished: |
Simple workaround for Relay bug (see: facebook/relay#1913). I have no idea how to write tests for this part of application. I would prefer to merge this and think about later since we will need to refactor it a little bit because of other tests...
0b0c2d1
to
4a9f694
Compare
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.
I also noticed that we create a new relay environment for every call of commitMutation and in the render method of QueryRenderer. Isn't the environment supposed to be a single instance for the whole application?
@@ -11,7 +11,8 @@ Try it in [Expo](https://expo.io/): | |||
* [Project structure](#project-structure) | |||
* [Working with GraphQL API](#working-with-graphql-api) | |||
* [Offline first](#offline-first) | |||
* [Best Practices](#best-practices) | |||
* [Error handling](#error-handling) | |||
* [Best practices](#best-practices) |
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.
Capital H, capital P.
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.
Would you prefer first capitals everywhere? I did this change intentionally but I also prefer first capitals everywhere in English headings...
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.
Now that I tried to find some arguments, I'm not so sure anymore. There are two main standards - title case and sentence case. Until now I thought the title case is more "correct", but apparently the sentence case is also a valid option. Maybe we can skip this decision for now and continue using the sentence case.
https://en.wikipedia.org/wiki/Letter_case#Headings_and_publication_titles
|
||
This should be considered as a valid full response and there should not be any errors. There may be nullable fields, however. | ||
|
||
2. GraphQL API returns `data = null` and `errors` field |
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.
I'm trying to test this scenario by modifying the fetchQuery:
const fetchQuery = async (operation, variables, cacheConfig) => {
return {
data: null,
errors: [
{
message: 'Some message',
},
],
};
Then I get the following message, is this the expected behavior? Or am I testing it wrong (and how can I test it correctly)?
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.
Yes, this is correct behavior. Data cannot be null
because this indicates a fatal GraphQL error and not partial error. See documentation. This is the only situation when Relay actually works correctly and handles errors...
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.
Ah, you are pointing to the docs. So yes, this is GeneralError
component in action.
Sounds like a mistake but that's not relevant here. I should fix it though. Probably during refactoring of this section, not now. You can create a new issue... |
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.
OK then.
Issue created #101 |
Simple workaround for Relay bug (see: facebook/relay#1913).
I have no idea how to write tests for this part of application.
I would prefer to merge this and think about it later since we will
need to refactor it a little bit because of other tests...
Related issue: #98
Checklist:
cacheConfig.force=true
is used