Strict typing with TS

[mike-marcacci]

This issue is to track a set of goals for type safety in TypeScript. In experiments, I have confirmed that all of these should be possible. While the increased verboseness has some ergonomic cost, the resulting type safety is overwhelmingly worth it in my opinion.

Object Types

For every field of a GraphQLObjectType, the resolver (including a "default resolver") must return a value compatible with the "source" of the field's GraphQL "type". For example, if ChildType expects { foo: string }, then the result of ParentType's child field cannot be { foo: 123 } or "foo".

Arguments & Input Types

The configuration of all defined arguments and input types must be checked against the types used in a resolver. For example, a resolver expecting args.foo to be a non-null string cannot be used in a field that defines argument foo as optional.

Scalars

The memory type of both custom and built-in scalars must be tracked. For example, a field with an argument configuration describing foo as a GraphQLString cannot have a resolver that expects foo to be a boolean.

---

Using generics and conditional types, I have been able to demonstrate that each of these is possible. However, as these effect the type signature of all primitive GraphQL components, there are a substantial number of changes across the repo. At one point I had opened a PR to DefinitelyTyped that gave partial support for strict typing of arguments, but the change was breaking (from the perspective of types) and I was too busy to effectively convey the significance before the PR was auto-closed.

As this is a natural time to implement this kind of change (during an existing push to re-type the whole codebase), I'm going to open a PR that converts the entire repo to TS in a way that accomplishes these goals. I'll begin my changes as soon as #2139 gets merged and there's a feature lock for 15.

[mike-marcacci]

As a tiny demonstration of one piece of this, consider this partial TS reimplementation of the graphql-js codebase:

// This is a reimplementation of GraphQLScalarType with generics.
class GraphQLScalarType<TInternal, TExternal> {
  internal: TInternal;
  external: TExternal;
}

// Here are two of our built-in scalars.
const GraphQLString = new GraphQLScalarType<string, string>();
const GraphQLInt = new GraphQLScalarType<number, number>();

// We want to extract the type, using the configuration object in the `type`
// field as our source of truth. If it's set to `GraphQLString`, then we want to
// make sure that the resolver in fact returns a string.
//
// In the real-world, there are many more factors (complex types, nullable
// types, lists, default resolvers, async resolvers, etc) but this is just a
// minimal example.
type FieldConfig<TConfig> = TConfig extends GraphQLScalarType<
  infer TInternal,
  any
>
  ? {
      type: TConfig;
      resolver: () => TInternal;
    }
  : never;

// Our dummy reimplementation of `GraphQLObjectType` can infer the field types
// from the config object, so we don't actually have to pass in this generic
// parameter.
class GraphQLObjectType<
  T extends {
    [K in keyof T]: T[K] extends { type: infer V } ? FieldConfig<V> : never;
  }
> {
  constructor(config: { name: string; fields: T }) {
    // Setup happens here...
  }
}

Using this, it's perfectly legal to create the following Person type:

const Person = new GraphQLObjectType({
  name: "Person",
  fields: {
    name: { type: GraphQLString, resolver: () => "hello" },
    age: {
      type: GraphQLInt,
      resolver: () => 12345
    }
  }
});

However, if we change the age resolver to return a string, our TypeScript will report an error, and prevent us from ever shipping bad code!

[xialvjun]

Make interface DocumentNode generic, like DocumentNode<Result, Variable>, so many community tools can use it.

For example:
@apollo/hooks can just useQuery(gqls.xxx_document) instead of useQuery<Result, Variable>(gqls.xxx_document), and the type of gqls.xxx_document can be declared by graphql-code-generator

[koistya]

How would you reference a type in its own initializer?

[Janpot]

By giving it a type annotation?

export const BooType: GraphQLObjectType<any, Context, any> = new GraphQLObjectType<any, Context, any>(...)

[MatthiasKunnen]

Does type: () => BooType work?

[mike-marcacci]

Great question! @Janpot is correct - in most cases the type parameters can be inferred from the configs, providing type safety without explicitly declaring types. However, it's certainly possible to be explicit with these parameters (which is necessary in certain cases like this). TypeScript will treat any explicit parameters as canon, and will use them to validate matching fields configs.

[dotansimha]

I recently opened a PR for using type inference in DocumentNode to allow clients to automatically infer types from generics: #2728
I think if we'll go with this one, we can always try to use the inference/generics for similar improvements.

[pspeter3]

Is this the right issue to follow for TypeScript type safety? I'm surprised by the usage of any in the resolver functions.

[dotansimha]

Is this the right issue to follow for TypeScript type safety? I'm surprised by the usage of any in the resolver functions.

I think it's definitely the right place for that :)

So at the moment, the any is used broadly, and generics are not really part of the type's implementation. I'm not sure that it worth the effort to fix that on the existing code since it's based on .d.ts enriching the Flow types, and not real TS.

There is active work to migrate the codebase itself to be TypeScript, and from my point of view, this could be the first step for better TS.

The second step could be an improvement of type-safety at the schema level, so if you are using the class-based form of creating a schema, you'll be able to get improved type-safety. TBH, I'm still not sure on what level we can get here, but we can definitely use more unknown and more generics, and allow some level of inference for scalars.

[pspeter3]

I think just being able to provide the arguments as generics as opposed to any would be a win. I also think having a TypeScript first approach would help.

[mike-marcacci]

Is this the right issue to follow for TypeScript type safety? I'm surprised by the usage of any in the resolver functions.

Definitely 😄

There is active work to migrate the codebase itself to be TypeScript, and from my point of view, this could be the first step for better TS.

This is my take too. I began implementing the strong types I proposed above in a refactor to TS... but the number of changes were truly overwhelming, and impossible for any reviewer to track. Instead, I decided to wait on this heroic undertaking to land. The goal there was to migrate the codebase while minimally changing its public TS interface. Once this is complete and merged, I plan on conducting a substantial overhaul of these types to enable the kind of functionality I described above.

[zachasme]

For inspiration there exists https://github.com/sikanhe/gqtx which is a thin layer over graphql-js which provides type-safety.

Also @ephemer appears to have a similar wrapping in the works here: #2104 (comment)

[jdpst]

@mike-marcacci That sounds terrific, I'd be very excited to see those changes. Now that v16 has dropped, do you have any sort of time frame in mind?

As @zachasme mentions, gqtx has done an awesome job in providing this (and even includes the relay-compliant helper functions). If we could have type-safe resolvers in this package, as in gqtx, I think many of us would give up the SDL/code-generator workaround.

Edit: I should add that I'd be happy to help with such an undertaking if help is required.

[mike-marcacci]

Hi @jdpst - Sorry for the communication delay here. It ended up taking much longer than expected for v16 to land, and I missed much of my available window to crank this out. However, this remains a bit of a pet project for myself... and these days I don't get much time to work on complex software changes, so it retains quite a personal appeal to me.

So I am still planning to do this "in my free time™" but would completely welcome somebody else doing it first. 😉

[eezing]

This may need a separate issue, but wanted to bring it up here first.

Was messing around with type inference and stumbled on this:

Looks like GraphQLNonNull and GraphQLList need a discriminator?

[yaacovCR]

@mike-marcacci I know your time is limited, but is the comment by @eezing above a blocker for your general work.

In general, I have a vague goal of introducing discriminators (well-known symbols) for all the types and using memoized predicates rather than instanceof to check the types at runtime. That would solve @eezing concern but will probably be a long time coming, if ever. I am curious, however, if the issue of wrapping types currently lacking a discriminators blocks #2188 and so the above changes (or a limited subset of them) would have to go first.

[CSNWEB]

I just ran into this issue, there definitely needs to be some discriminator, I just added a prop with a string literal to both.

[josiah-roberts]

This has plagued me across multiple codebases. Now one of the first things I do in a new repo is

import { GraphQLType } from "graphql";

declare module "graphql" {
  interface GraphQLNonNull<T extends GraphQLType> {
    _brandNonNull: "GraphQLNonNull";
  }

  interface GraphQLList<T extends GraphQLType> {
    _brandList: "GraphQLList";
  }
}

This is really unfortunate. If the team would be open to PRs, I will simply make a PR that fixes this on the type level. GraphQLNonNull should not be assignable to GraphQLList.

[mike-marcacci]

Just as an update, I finally found some time to start implementing this in April, and I've got all my initial changes in this commit.

There were zero run-time changes required, but types underwent a major refactor in ways that cannot be backwards-compatible. I think this is fine, and the benefits are so massive that the change cost for consumers is (IMO) completely justified.

However, I did run into a big blocker:

export type GraphQLFieldConfigMap<
  TSource,
  TContext,
  TFields extends BaseReadonlyMapType,
> = {
  [K in keyof TFields]: GraphQLFieldConfig<
    TSource,
    TContext,
    // TODO: Once TS supports the ability to infer from the type of a property,
    // this `any` should be removed. See:
    // https://github.com/microsoft/TypeScript/issues/26242
    // https://github.com/microsoft/TypeScript/pull/26349
    // https://github.com/microsoft/TypeScript/issues/32794
    // https://github.com/microsoft/TypeScript/issues/42388
    // https://gist.github.com/shicks/219a081b74df7ad28e683761f51102f1
    any,
    TFields[K],
    K
  >;
};

I need additional support from TypeScript to allow the TArgs parameter of GraphQLFieldConfig to be inferred when defining GraphQLFieldConfigMap.

It's still possible to use this by fully defining the type of each GraphQLFieldConfig in the map, but this is not ergonomic and the any defeats argument type safety for any value not explicitly defined.

With these defects, I'm skeptical that I could build sufficient momentum in the community to actually make this part of a release.

If TypeScript adds support for this, I'll finish the changes and champion the rollout. The last time I looked, the most promising proposal was:

• Proposal: Partial Type Argument Inference microsoft/TypeScript#26242
• Implement partial type argument inference using the _ sigil microsoft/TypeScript#26349

If anyone watching this is an active participant in TypeScript's evolution, let me know how I can better push for this functionality.

EDIT: after trying to update the above stagnated TypeScript PR, I discovered that is isn't quite sufficient for this use-case. I've started working on a concrete proposal and familiarizing myself with the TypeScript compiler, with the hope of supporting the infer keyword in place of generic values. This turns out to be a pretty substantial change, though, so it might be more practical to come up with a "special case" solution. Either way, the following is a simplification of our use-case that preserves the intent:

// This describes "internal consistency" constraints.
type InternallyConsistentValue<T> = {
  a: T
  b: T
}

// Each "value" in the map can have a different type T,
// but each must be internally consistent.
type HeterogeneousMap = {
  [key: string]: InternallyConsistentValue<infer>
}

const success: HeterogeneousMap = {
  foo: {a: "hello", b: "world"},
  bar: {a: 1, b: 2},
};

const failure: HeterogeneousMap = {
  // @ts-expect-error
  foo: {a: "hello", b: 2},
};