Skip to content

GraphQLObjectTypeConfig<TSource> to GraphQLFieldResolveFn<TSource, TResult> #425

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

GraphQLObjectTypeConfig<TSource> to GraphQLFieldResolveFn<TSource, TResult> #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b22ef58
make GraphQLFieldResolveFn to GraphQLFieldResolveFn<TSource, TResult>
iamchenxin Jul 8, 2016
784cd09
amend code style
iamchenxin Jul 25, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 41 additions & 5 deletions src/__tests__/starWarsData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
/* @flow */
/**
* Copyright (c) 2015, Facebook, Inc.
* All rights reserved.
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

/**
* This defines a basic set of data for our Star Wars Schema.
*
Expand All @@ -13,6 +13,8 @@
* JSON objects in a more complex demo.
*/

import invariant from '../jsutils/invariant';

const luke = {
id: '1000',
name: 'Luke Skywalker',
Expand Down Expand Up @@ -80,6 +82,34 @@ const droidData = {
'2001': artoo,
};

/**
* These are data types corresponding to the schema design.
* They represent what the input and output datas should be
* in schema's resolving hierarchy.
* If the schema is re-designed,these types should be re-designed too.
*/
export type Character = {
id: string,
name?: string,
friends?: Array<string>,
appearsIn?: Array<number>,
};

export type Human = {
id: string,
name?: string,
friends?: Array<string>,
appearsIn?: Array<number>,
homePlanet?: string,
};

export type Droid = {
id: string,
name?: string,
friends?: Array<string>,
appearsIn?: Array<number>,
primaryFunction?: string
};
/**
* Helper function to get a character by ID.
*/
Expand All @@ -91,14 +121,20 @@ function getCharacter(id) {
/**
* Allows us to query for a character's friends.
*/
export function getFriends(character) {
export function getFriends(character: Character): Array<Promise<Character>> {
// as you can see,with Flow
// If you choose a schema structure like {friends?: Array<*>}.
// You must check it ,or re-design your schema.
// Flow will keep you away from hiden bugs.
invariant(character.friends !== undefined,
'Character.friends should not be undefined');
return character.friends.map(id => getCharacter(id));
}

/**
* Allows us to fetch the undisputed hero of the Star Wars trilogy, R2-D2.
*/
export function getHero(episode) {
export function getHero(episode: number): Character {
if (episode === 5) {
// Luke is the hero of Episode V.
return luke;
Expand All @@ -110,13 +146,13 @@ export function getHero(episode) {
/**
* Allows us to query for the human with the given id.
*/
export function getHuman(id) {
export function getHuman(id: string): Human {
return humanData[id];
}

/**
* Allows us to query for the droid with the given id.
*/
export function getDroid(id) {
export function getDroid(id: string): Droid {
return droidData[id];
}
15 changes: 9 additions & 6 deletions src/__tests__/starWarsSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* @flow */
/**
* Copyright (c) 2015, Facebook, Inc.
* All rights reserved.
Expand All @@ -19,6 +20,8 @@ import {

import { getFriends, getHero, getHuman, getDroid } from './starWarsData.js';

import type { Character, Human, Droid } from './starWarsData.js';

/**
* This is designed to be an end-to-end test, demonstrating
* the full GraphQL stack.
Expand Down Expand Up @@ -132,7 +135,7 @@ const characterInterface = new GraphQLInterfaceType({
},
}),
resolveType: character => {
return getHuman(character.id) ? humanType : droidType;
return getHuman((character: any).id) ? humanType : droidType;
}
});

Expand Down Expand Up @@ -164,7 +167,7 @@ const humanType = new GraphQLObjectType({
type: new GraphQLList(characterInterface),
description: 'The friends of the human, or an empty list if they ' +
'have none.',
resolve: human => getFriends(human),
resolve: (human: Human): Array<Promise<Character>> => getFriends(human),
Copy link
Contributor

Choose a reason for hiding this comment

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

are these resolve function type hints necessary? I would expect that Flow would not require them since you already typed getFriends

Copy link
Contributor Author

@iamchenxin iamchenxin Jul 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it is necessary, the input and output datatype should be a part of schema's design. A explicit declare give the resolver a constraint.(Makes it clear to other people, what data should be in and what data should be out).Will explain this below.

},
appearsIn: {
type: new GraphQLList(episodeEnum),
Expand Down Expand Up @@ -214,7 +217,7 @@ const droidType = new GraphQLObjectType({
type: new GraphQLList(characterInterface),
description: 'The friends of the droid, or an empty list if they ' +
'have none.',
resolve: droid => getFriends(droid),
resolve: (droid: Droid): Array<Promise<Character>> => getFriends(droid),
},
appearsIn: {
type: new GraphQLList(episodeEnum),
Expand Down Expand Up @@ -261,7 +264,7 @@ const queryType = new GraphQLObjectType({
type: episodeEnum
}
},
resolve: (root, { episode }) => getHero(episode),
resolve: (root, { episode }): Character => getHero((episode: any)),
},
human: {
type: humanType,
Expand All @@ -271,7 +274,7 @@ const queryType = new GraphQLObjectType({
type: new GraphQLNonNull(GraphQLString)
}
},
resolve: (root, { id }) => getHuman(id),
resolve: (root, { id }): Human => getHuman((id: any)),
},
droid: {
type: droidType,
Expand All @@ -281,7 +284,7 @@ const queryType = new GraphQLObjectType({
type: new GraphQLNonNull(GraphQLString)
}
},
resolve: (root, { id }) => getDroid(id),
resolve: (root, { id }): Droid => getDroid((id: any)),
},
})
});
Expand Down
2 changes: 1 addition & 1 deletion src/execution/execute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ function resolveField(
// Isolates the "ReturnOrAbrupt" behavior to not de-opt the `resolveField`
// function. Returns the result of resolveFn or the abrupt-return Error object.
function resolveOrError(
resolveFn: GraphQLFieldResolveFn<*>,
resolveFn: GraphQLFieldResolveFn<*, *>,
source: mixed,
args: { [key: string]: mixed },
context: mixed,
Expand Down
8 changes: 4 additions & 4 deletions src/type/definition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,12 +478,12 @@ export type GraphQLIsTypeOfFn = (
info: GraphQLResolveInfo
) => boolean

export type GraphQLFieldResolveFn<TSource> = (
export type GraphQLFieldResolveFn<TSource, TResult> = (
source: TSource,
args: {[argName: string]: mixed},
context: mixed,
info: GraphQLResolveInfo
) => mixed
) => TResult
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if this actually aids in Flow's type checking? What issue would Flow not catch that it does catch after this change? I'd love to see an example so I can understand better.

Copy link
Contributor Author

@iamchenxin iamchenxin Jul 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Im not sure what the sentence What issue would Flow not catch that it does catch after this change? means. My english is poor.

If it means What will be happened if TSource -> any be fixed in the future. The explanation is:
The resovler's hierarchy is something like this:

ROOT -> A
  A -> B
    B -> BF
        BF -> KK
    B -> B2
  A -> C
    C -> E
    C -> G

Ideally the TResult can not be typed ,because the Lower level's input type is Upper level's output. The best solution is only needed to type the input for every resovler, but type the result is not bad.
Because at now ,in inner code TSource is casted to any. So TResult is a alternative solution,to link the type system from ROOT to leaf.( A -> B -> BF -> KK).
If in the future, a new PR make the TSource type rightly pass to its children. The explicit type for result will be no longer needed. and in the user's code the explicit typed result will be a redundancy. But the redundancy will not make any problem,it is only a redundancy(Because B is B).

Copy link
Contributor Author

@iamchenxin iamchenxin Jul 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

If What issue would Flow not catch that it does catch after this change? means what is the usage of TResult.
With TResult the Type system can check the output for a resovle, and ensure it is corresponding to the lower level .There is a example:
a demonstration for a Relay's Connection constraint With the TResult, the type system will check the returned data for the user's output,if the user typo, like 'id' -> 'idd'. flow will give errors.(User must return a ConnectionJS<*>)
untitled


export type GraphQLResolveInfo = {
fieldName: string;
Expand All @@ -501,7 +501,7 @@ export type GraphQLResolveInfo = {
export type GraphQLFieldConfig<TSource> = {
type: GraphQLOutputType;
args?: GraphQLFieldConfigArgumentMap;
resolve?: GraphQLFieldResolveFn<TSource>;
resolve?: GraphQLFieldResolveFn<TSource, *>;
deprecationReason?: ?string;
description?: ?string;
}
Expand All @@ -525,7 +525,7 @@ export type GraphQLFieldDefinition = {
description: ?string;
type: GraphQLOutputType;
args: Array<GraphQLArgument>;
resolve?: GraphQLFieldResolveFn<*>;
resolve?: GraphQLFieldResolveFn<*, *>;
deprecationReason?: ?string;
}

Expand Down