Skip to content

Conversation

iamchenxin
Copy link
Contributor

@iamchenxin iamchenxin commented Jul 8, 2016

GraphQLObjectTypeConfig<TSource> to GraphQLFieldResolveFn<TSource, TResult>
Make user can do a whole top-bottom Flow check between their resolvers.
With the Flow typed feature, user can ensure his data structure is correspond to schema . ex: starWarsData.js L103

More details see the src/tests/starWarsSchema.js

A simple demonstration:

  type DCPost;
  type DComment;
  type DUser;
  const Post = new GraphQLObjectType({
    name: 'Post',
    fields: () => ({
      ...
      comment: {
        type: new GraphQLList(Comment),
        resolve: (src:DPost):DComment[] => { // Flow check DPost -> DComment[]
          return ...;
        } }
    }),
  });

  const Comment = new GraphQLObjectType({
    name: 'Comment',
    fields: () => ({
      ...
      user: {
        type: User,
        resolve: (src:DComment):DUser => { // DComment -> DUser
          return ...;
        }
      },
    }),
  });

  const User = new GraphQLObjectType({
      ...
  });

Note: Because in v0.61 ,in source code use a any type to force cast TSource. So it is user's responsibility to check the Data Type is not typo. In above code , if resolve: (src:DPost):DComment[] is typo to resolve: (src:DPost):DSomeAnotherType[](and user return a wrong data), Flow will still pass. Flow will not check DSomeAnotherType is DComment,cause of DSomeAnotherType -> any -> DComment

@ghost ghost added the CLA Signed label Jul 8, 2016
@iamchenxin iamchenxin force-pushed the GraphQLFieldResolveFn branch from 046cbba to e2dc75c Compare July 8, 2016 12:58
@ghost ghost added the CLA Signed label Jul 12, 2016
node_modules
coverage
dist
.vscode
Copy link
Contributor

Choose a reason for hiding this comment

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

could you remove this to keep this PR single-purpose?

@leebyron
Copy link
Contributor

Thanks for your hard work on this! I've left some comments inline, but please reduce the scope of this to only the changes necessary for this change - otherwise it's difficult to follow what was necessary and what was not

@iamchenxin iamchenxin force-pushed the GraphQLFieldResolveFn branch 2 times, most recently from 2701c60 to 3de66da Compare July 19, 2016 10:46
@@ -1,10 +1,12 @@
/* @flow */
/* eslint quote-props: ["error", "as-needed",
{ "keywords": true, "unnecessary": false }]*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to fix Flow error: non-string literal property keys not supported, like:

const droidData = {
  2000: threepio,
  2001: artoo,
};

Copy link
Contributor

@leebyron leebyron Jul 20, 2016

Choose a reason for hiding this comment

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

Ah, thanks for the explanation!

I think you actually want {numbers: true} rather than keywords to handle this case.

I just landed a change which applies this across the whole project so that we would not need this special case, so now it can be removed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its cool, will remove them.

@ghost ghost added the CLA Signed label Jul 19, 2016
@iamchenxin
Copy link
Contributor Author

iamchenxin commented Jul 19, 2016

Amended code style.
Separate .gitignore to #437 .

leebyron added a commit that referenced this pull request Jul 20, 2016
Flow does not allow unquoted numeric object properties, so ensure ESLint and Flow agree on this.

As originally illustrated in #425
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

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.

@ghost ghost added the CLA Signed label Jul 20, 2016

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

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

Choose a reason for hiding this comment

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

stylistic issue: please use spaces inside the { } in import to match the others in the project

@iamchenxin iamchenxin force-pushed the GraphQLFieldResolveFn branch from 3de66da to 784cd09 Compare July 25, 2016 14:40
@ghost ghost added the CLA Signed label Jul 25, 2016
@iamchenxin iamchenxin closed this Sep 19, 2016
@iamchenxin iamchenxin reopened this Sep 19, 2016
@ghost ghost added the CLA Signed label Sep 19, 2016
leebyron added a commit that referenced this pull request Nov 16, 2016
leebyron added a commit that referenced this pull request Nov 16, 2016
This cleans up the top level tests and flow-types them. It also moved one unrelated test into a more appropriate location.

Inspired by #425
leebyron added a commit that referenced this pull request Nov 16, 2016
This cleans up the top level tests and flow-types them. It also moved one unrelated test into a more appropriate location.

Inspired by #425
@iamchenxin iamchenxin closed this Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants