Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Commit

Permalink
Handle partial GraphQL errors
Browse files Browse the repository at this point in the history
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...
  • Loading branch information
mrtnzlml committed Dec 28, 2017
1 parent 4dac580 commit 4a9f694
Show file tree
Hide file tree
Showing 11 changed files with 150 additions and 30 deletions.
24 changes: 21 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
* [Accessing arbitrarily nested, possibly nullable properties on a JavaScript object](#accessing-arbitrarily-nested-possibly-nullable-properties-on-a-javascript-object)
* [Known issues](#known-issues)
* [Troubleshooting](#troubleshooting)
Expand Down Expand Up @@ -114,7 +115,25 @@ this.setState({ refreshing: true }, () => {
});
```

## Best Practices
## Error handling

Error handling is complicated in general - especially in GraphQL environment. There are several scenarios that may occur:

1. GraphQL API returns `data` field and no `errors`

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

This is fatal error. Server was not able to get data and it's probably not operating correctly. It's like equivalent of total GraphQL server error (500). We should display full page error (`GeneralError` component).

3. GraphQL API returns `data` but also `errors` field

Most common scenario (somewhere between). In this case we are able to fetch at least something but it failed partially so there are errors and we can expect some nullable fields. This may be just missing prices but also completely missing data. It's very different to point 2.

We are showing little warning in this case. How to handle nullable fields really depends on the situation. Sometimes it's OK to leave it empty instead of for example hotel rating (★★★), sometimes it's necessary to display error message or sad picture in case of completely missing hotels. It depends. We are always trying to render as much as possible.

## Best practices

### Accessing arbitrarily nested, possibly nullable properties on a JavaScript object

Expand Down Expand Up @@ -142,7 +161,6 @@ idx(props, _ => _.user.friends[0].friends)
#### Important to fix before production ready state

- `PaginationContainer` fails for zero results returned: https://github.com/facebook/relay/issues/1852, fixed by https://github.com/facebook/relay/commit/a17b462b3ff7355df4858a42ddda75f58c161302 (not released yet)
- Relay swallows all GraphQL errors in `QueryRenderer`: https://github.com/facebook/relay/issues/1913

#### Improvements necessary for production usage

Expand Down
1 change: 1 addition & 0 deletions app/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export { default as Date } from './src/datetime/Date';
export { default as DateTime } from './src/datetime/DateTime';

export { default as GeneralError } from './src/errors/GeneralError';
export { default as PartialFailure } from './src/errors/PartialFailure';

export { default as DatePicker } from './src/forms/DatePicker';
export { default as TextInput } from './src/forms/TextInput';
Expand Down
16 changes: 14 additions & 2 deletions app/common/src/Color.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
const Color = {
// brand colors
// https://images.kiwi.com/content-media/kiwicom_brand_colours.pdf
brand: '#0097A9',
brandSecondary: '#0CB3C7',
brand: '#0097a9',
brandSecondary: '#0cb3c7',

icon: {
get grey() {
Expand All @@ -18,6 +18,18 @@ const Color = {

// other colors
// https://material.io/guidelines/style/color.html#color-color-palette
red: {
$100: '#ffcdd2',
$200: '#ef9a9a',
$300: '#e57373',
$400: '#ef5350',
$500: '#f44336',
$600: '#e53935',
$700: '#d32f2f',
$800: '#c62828',
$900: '#b71c1c',
},

grey: {
$100: '#f5f5f5',
$200: '#eeeeee',
Expand Down
37 changes: 37 additions & 0 deletions app/common/src/errors/PartialFailure.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// @flow

import * as React from 'react';
import { Text, View, StyleSheet } from 'react-native';
import { Color } from '@kiwicom/react-native-app-common';

type Props = {|
children: React.Node,
|};

export default function PartialFailure({ children }: Props) {
const style = createStyle();
return [
children,
<View key="warning" style={style.container}>
<Text>
Some parts of the page may be missing due to partial server error.
</Text>
</View>,
];
}

function createStyle() {
return StyleSheet.create({
container: {
flexDirection: 'row',
justifyContent: 'center',
position: 'absolute',
left: 0,
right: 0,
bottom: 0,
backgroundColor: Color.red.$100,
paddingVertical: 5,
paddingHorizontal: 10,
},
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`calculates the offsets base on icon size 1`] = `
<Icon
allowFontScaling={false}
color="#0097A9"
color="#0097a9"
name="map-marker"
size={50}
style={
Expand All @@ -19,7 +19,7 @@ exports[`calculates the offsets base on icon size 1`] = `
exports[`calculates the offsets base on icon size 2`] = `
<Icon
allowFontScaling={false}
color="#0097A9"
color="#0097a9"
name="map-marker"
size={100}
style={
Expand Down
5 changes: 0 additions & 5 deletions app/common/src/rating/Stars.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,5 @@ type Props = {|
|};

export default function Stars({ rating }: Props) {
if (!rating) {
console.warn(
'Rating cannot be undefined in Stars component. This usually indicates GraphQL API failure.',
);
}
return <Text>{'★'.repeat(rating || 0)}</Text>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ exports[`SearchForm Render all inputs 1`] = `
<View
style={
Object {
"backgroundColor": "#0CB3C7",
"backgroundColor": "#0cb3c7",
"padding": 10,
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ exports[`Filter button render default button 1`] = `
styles={
Object {
"button": Object {
"backgroundColor": "#0097A9",
"backgroundColor": "#0097a9",
"paddingLeft": 0,
},
"icon": Object {
"backgroundColor": "#0097A9",
"backgroundColor": "#0097a9",
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion app/relay/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type CommitMutationConfig = {|
|};

export const commitMutation = (config: CommitMutationConfig) =>
commitRelayMutation(createEnvironment(), config);
commitRelayMutation(createEnvironment(() => {}), config);

export type QueryRendererProps = {|
query: string,
Expand Down
60 changes: 49 additions & 11 deletions app/relay/src/Environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,23 @@ import RelayQueryResponseCache from './RelayQueryResponseCache';

const cache = new RelayQueryResponseCache();

export default function createEnvironment(accessToken: string = '') {
const store = new Store(new RecordSource());
type GraphQLError = {|
message: string,
locations?: {|
line: number,
column: number,
|},
path?: string[],
_proxy?: {|
statusCode: number,
url: string,
|},
|};

export default function createEnvironment(
onPartialError: GraphQLError => void,
accessToken: string = '',
) {
const networkHeaders: Object = {
Accept: 'application/json',
'Content-Type': 'application/json',
Expand All @@ -23,7 +37,7 @@ export default function createEnvironment(accessToken: string = '') {
if (cacheConfig.force === true) {
await cache.remove(query, variables);
} else {
// refetch not forced so try to find it in cache
// refetch is not forced so try to find it in cache
const value = await cache.get(query, variables);
if (value !== null) {
// cache hit, yay
Expand All @@ -41,16 +55,40 @@ export default function createEnvironment(accessToken: string = '') {
}),
})).json();

// always save the response (probably not needed during cache "force")
await cache.set(query, variables, jsonPayload);

if (!jsonPayload.errors) {
// always save the valid response (probably not needed during cache "force")
await cache.set(query, variables, jsonPayload);
}
return jsonPayload;
};

const network = Network.create(fetchQuery);
return new PartialErrorsEnvironment(
{
network: Network.create(fetchQuery),
store: new Store(new RecordSource()),
},
onPartialError,
);
}

/**
* This environment is workaround for: https://github.com/facebook/relay/issues/1913
*/
class PartialErrorsEnvironment extends Environment {
onPartialError: GraphQLError => void;

return new Environment({
network,
store,
});
constructor(config: Object, onPartialError: GraphQLError => void) {
super(config);
this.onPartialError = onPartialError;
}

execute = (executeConfig: Object) => {
return super.execute(executeConfig).do({
next: executePayload => {
if (executePayload.errors) {
executePayload.errors.map(error => this.onPartialError(error));
}
},
});
};
}
25 changes: 22 additions & 3 deletions app/relay/src/QueryRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { QueryRenderer as OriginalQueryRenderer } from 'react-relay';
import {
FullPageLoading,
GeneralError,
PartialFailure,
} from '@kiwicom/react-native-app-common';

import createEnvironment from './Environment';
Expand All @@ -17,26 +18,44 @@ type Props = {|
|};

export default class QueryRenderer extends React.Component<Props> {
partialError: Object;

createEnvironment = () =>
createEnvironment(partialError => {
this.partialError = partialError;
}, this.props.accessToken || '');

renderRelayContainer = ({
error,
props,
}: {
error: Object,
props: Object,
}) => {
// FIXME: Relay swallows all GraphQL errors
if (error) {
// total failure (data == null, errors != null)
return <GeneralError errorMessage={error.message} />;
} else if (props) {
}

if (props) {
if (this.partialError) {
// partial failure (data != null, errors != null)
// Relay swallows all GraphQL errors if 'data != null' (see: https://github.com/facebook/relay/issues/1913)
return <PartialFailure>{this.props.render(props)}</PartialFailure>;
}

// success (data != null, errors == null)
return this.props.render(props);
}

// no data or errors yet
return <FullPageLoading />;
};

render = () => {
return (
<OriginalQueryRenderer
environment={createEnvironment(this.props.accessToken || '')}
environment={this.createEnvironment()}
query={this.props.query}
variables={this.props.variables}
render={this.renderRelayContainer}
Expand Down

0 comments on commit 4a9f694

Please sign in to comment.