Export flow types from index.js

[leebyron]

This adds all flow types exported from individual files to relevant index.js files as well.

This enables those using graphql.js with flow to write something like:

import type { ... } from 'graphql'

[leebyron]

Note: this contains a breaking change!

Within field resolvers, info.fieldASTs has been renamed to info.fieldNodes

[nodkz]

Test 0.8.0-beta1 in our project.
All our tests passed. And works perfectly.

[taion]

This seems like a bad idea to me.

A lot of these Flow types do not seem to be set up in a way that makes them usable outside of this library.

Consider GraphQLFieldConfig<TSource> and GraphQLFieldResolver. GraphQLFieldConfig is templated only on TSource, but not on TArgs, or on the various types for context and info.rootValue.

As such, it's quite inconvenient to get any sort of type checking with GraphQL schemas that use Flow – essentially it'd require large amounts of coercion to and from any for uses of args, or else some layer on top of what's available here.

I would strongly recommend backing out the update to include the Flow types. As set up right now, I don't see a good way to make use of these typings. I don't think there's a trivial fix available either – it looks like it's going to take some effort to rethink the typings in such a way that they work well for users of this library.

[leebyron]

:/ I would really hope for this kind of feedback during the beta phase, it's too late to back out of this release.

At this point I'm open to ideas on how to make the typings more useful going forward. Should we type some more complex things as any? Are there specific improvements we can make to the types?

[taion]

I don't think removing Flow annotations is semver-breaking. They shouldn't cause any additional errors for users of this library.

Short of typing everything as any, I think the right way to do things would be:

• Template schemas on context
• Template fields on args and context in addition to obj/source
• Template the top-level graphql function and other execution functions on the type of context, and require that they accept a schema templated on the same context type
• Figure out what to do with rootValue typing (???)

But I really don't know. I wasn't able to update my GraphQL schema because I have a dependency on graphql-relay, which didn't have betas available – but even just attempting to update graphql-relay to work with the new Flow exports exposes some of the shortcomings with the current types.

[leebyron]

Thanks for the feedback. It sounds like there are two categories of users of types with different needs. One group is looking for types to make guarantees about their usage of the functions exported by GraphQL.js, others are looking for types to make guarantees about their resolver functions being correct. The former seems happy with the change, the latter seems unhappy. Removing types completely will make the former group unhappy. So I think the right thing to do now is just to add any types within the resolver functions where there are clear shortcomings - that's functionally the same as having no flow types at all for those cases. The future addition of better types can be breaking. I like your ideas on type improvements though, we'll see how many of those we can accomplish first.

[taion]

Though in a perfect world – and this might even be possible with $ObjMap magic – the shape of args for the resolver should be inferred from the args property on GraphQLFieldConfig...

I think regardless there's a lot of thought that's going to be necessary to expose type definitions that would be useful for defining schemas – enough to be a fairly significant project. They're not necessarily going to be the same kind of type definitions that would be useful for this package internally. They're somewhat difficult even for other libraries that consume this library.

[stubailo]

I feel like the actual final solution probably involves some code generation to either create the schema argument types from the flow types, or the other way around.

[taion]

I think the issue is that if you type e.g. the args argument to resolvers as any, you lose the benefit of having type checking inside this library – in principle someone could do something un-type-safe there.

I don't see how you can just use the functions exported by GraphQL.js unless you are doing so entirely outside of the context of defining a schema. The idea of applying a minimal type check to calls to graphql() but not type-checking my actual schema sounds like a "why would you do this?" kind of thing – especially if I'm going to be using something like express-graphql anyway.

I don't think the tradeoff of having type checks for calls to new GraphQLObjectType but with no effective checking on the resolver function is worth loosening up types inside this library and possibly losing type safety there.

[taion]

@stubailo

$ObjMap is, like, magic: https://github.com/facebook/flow/blob/master/tests/objmap/objmap.js

It's like actual C++ template stuff.

I wish it were documented.

[leebyron]

resolvers as any, you lose the benefit of having type checking inside this library – in principle someone could do something un-type-safe there.

That was the current state of things before adding Flow types. So my point here is that rather than removing them all-together we can just weaken them where they're insufficient. Adding back typings later can be considered breaking changes.

I don't see how you can just use the functions exported by GraphQL.js unless you are doing so entirely outside of the context of defining a schema.

Exactly this. It's not an uncommon use case, especially if you're building other libraries for doing things with GraphQL.

[taion]

Sorry, what I mean specifically is that loosening up the typings reduces the type-safety of code within this library. As a consumer of this library, I'd rather not be exposed to potential type safety errors within the graphql-js library that might arise from re-typing things from mixed to any, especially since those types don't give me any additional safety in my own code.

Also, the use case for those libraries only applies to ones that are entirely agnostic to the underlying schema. This doesn't even apply for cases like graphql-relay-js that expose field definition helpers like https://github.com/graphql/graphql-relay-js/blob/v0.4.3/src/node/plural.js#L50-L55.

[leebyron]

Also, the use case for those libraries only applies to ones that are entirely agnostic to the underlying schema.

That's not quite true. It applies to those libraries which are just not defining the resolver functions for underlying schema. Libraries that use a schema still benefit from this kind of typing and was the primary motivator for adding them.

[leebyron]

Please correct me if I'm off-base here, but I'm understanding your feedback to be specific to the type safety of field resolver functions, is that correct?

[taion]

That's the first example that came to mind. I was taking a stab at updating graphql-relay-js, but stopped when I realized that making the resolver functions pass the Flow check would have required casting through any.

Regarding libraries that use schemas – would it be possible to just stop exporting the typings for schema type definitions, and only retain those exports that are useful for those use cases? e.g. export the signature for graphql(), but not for GraphQLObjectType.

[leebyron]

At some point you need to bridge between the typed and untyped worlds. That would mean exporting a signature for graphql() (and then likely also parse(), execute() and validate()) that acceptedany` instead of something well typed.

That means at some-point, somewhere, there will be a cast through any. It will either be explicit in user code, or it will be implicit, within the graphql.js library. I think moving it into the library is a fine move for improved developer experience.

I'm not sure if removing flow types for GraphQLObjectType is the answer here since that may create inconsistencies.

I'll start by looking into improving the resolver methods and then removing the need for casting through any as much as possible while retaining as much strong typing as we can.

I think that definitely means that there will be opportunity to improve the type safety of field resolver functions over time, and those likely will require breaking change version bumps - but at least we can start from a place where we have some typing.

[taion]

Fair enough – in practice I can't really imagine any bugs that could potentially arise in graphql-js from typing e.g. args, context, and rootValue as any. It's take something really weird. Just something to watch out for I guess.

[leebyron]

Certainly no new kinds of bugs that we wouldn't have seen in a version before Flow types at all.

I can also work with the Flow team in coming up with ways to more correctly type what we have.

[rjkmurray]

Review