Allow isTypeOf and resolveType to return a Promise

[dherault]

Hello,

The idea behind this PR is to allow GraphQLObjectType#isTypeOf, GraphQLInterfaceType#resolveType and GraphQLUnionType#resolveType to return a Promise.

The PR consists of:

• Changes to execute.js to support the new promise feature.
• New tests: Promise.resolve duplicates of the existing ones about isTypeOf and resolveType + new ones to test the cases where the promise is rejected.

This relates to issue #398. Use case is the following:
For some projects fetching data can be expensive. To prevent that, the 'source' passed to resolveFns can be thought as an ID string referencing remote data, so the fields resolve only what is needed based on that ID. It works fine, except when abstract types require a sync resolving of the type behind an ID string.
The use case at my company: We create our GraphQL schema based on our RDF ontology, query with SPARQL, etc... (see this almost open-sourced project: semantic-graphql)

[sonewman]

instead of always wrapping the result of these function calls with a promise, i think it would be better to check the return value is thenable that way for the average use case the user is not penalised on the extra (albeit minor) performance characteristics of promise allocations for all type resolution.

[dherault]

You're right, I'll see about it

[dherault]

It's done. It allows fully synchronous abstract type resolution like before.
PS: I've rebased my PR so this diff is outdated

[leebyron]

Thanks so much for digging into this!

I'll do a more thorough review, thanks for your patience :)

[leebyron]

This is looking great! Some suggestions inline

[leebyron]

Similarly here - can we avoid the per-field creation for a Promise for the more typical case of returnType.isTypeOf returning synchronously?

[dherault]

Done. Had to create a new function to handle the common code between sync and async, thus increasing complexity a bit.

[leebyron]

Suggestion: move this to the bottom of the file for less entropy in the diff.

Also, instead of having a default value for arguments, you could keep the defaultResolveTypeFn function, but just have it collect the possibleTypes and then call this function.

[leebyron]

I'm a bit concerned about this function calling itself recursively. There are cases where a great many types can be iterated through here and the recursion could cause significant overhead. Also, if the matching type happens to be last in a long list of possible types, then it would have to wait for each promise to resolve sequentially before a match could be found.

I suggest calling the isTypeOf functions synchronously such that any promises generated run in parallel rather than in series. That way, if any synchronously return true, then the function may return synchronously, even if some of the isTypeOf functions return Promises. That also preserves the existing behavior. However if some responses were thenable then you could use Promise.all to resolve them in parallel and then iterate through the results to determine a match.

[dherault]

Done. Here are some thoughts on the new implementation.

• The ordering of types is not preserved when mixing sync and async isTypeOf:

const Dog = new GraphQLObjectType({
  isTypeOf: () => Promise.resolve(true),
  /* ... */
});

const Cat = new GraphQLObjectType({
  isTypeOf: () => true,
  /* ... */
});

const PetType = new GraphQLUnionType({
  types: [Dog, Cat]
  /* ... */
});

In a fully-sync schema, Dog would be picked as the correct type for any query on Pet, but in the proposed implementation, we resolve the sync ones first, so Cat is picked. In a fully async schema the ordering is preserved. (Keeping that ordering was what I was trying to achieve using the outdated recursive function)

• Error handling changed a bit. See this diff:

const DogType = new GraphQLObjectType({
  isTypeOf: source => Promise.resolve(source instanceof Dog),
  /* ... */
});

const CatType = new GraphQLObjectType({
  isTypeOf: () => Promise.reject('Oh no!'),
  /* ... */
});

const PetType = new GraphQLUnionType({
  types: [Dog, Cat]
  /* ... */
});

When querying for a Dog everything should be alright but since we run Promise.all on all async isTypeOf functions it throws the Cat error. Maybe that is not an issue since rejecting in isTypeOf would never be the developer's intent.

[sonewman]

I have a quick question, if one of these functions resolved true, but another one returns a rejected promise then the this will completely fail. Is this the expected behaviour?

While I think resolving all in parallel is a good idea, I was just thinking if the typeOf methods involve individual database lookups or something similar then those kinds of implementations would be restricted to using the resolveType option to control synchronicity.

[dherault]

I have a quick question, if one of these functions resolved true, but another one returns a rejected promise then the this will completely fail. Is this the expected behaviour?

I agree the new behavior is not optimal (see previous comments). Any thoughts on how to handle it are welcome.

While I think resolving all in parallel is a good idea, I was just thinking if the typeOf methods involve individual database lookups or something similar then those kinds of implementations would be restricted to using the resolveType option to control synchronicity.

Yes, but in a way, resolving types in the order info.schema.getPossibleTypes(abstractType) gives us is also arbitrary. So a "first-to-resolve" strategy would not be that bad.

[leebyron]

Re: the order of possible types: At Facebook we actually have a version of this resolver that runs in development environments (due to performance concerns) that runs through all possible types and makes an assertion that exactly one of them is the correct type.

[leebyron]

This is looking awesome, thanks! I'll do a bit more collaborative edits and then lets get this landed.