Rethink generated namespace/interface design for TypeScript

[schickling]

The current generated namespace/interface design for TypeScript for a User GraphQL type looks roughly the following:

export namespace UserResolvers {
  export const defaultResolvers = {
    id: (parent: User) => parent.id,
    // more fields backed by `User` model ...
  };

  export type IdResolver = (
    parent: User,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => string | Promise<string>;
  
  // more resolver types...

  export interface Type {
    id: (
      parent: User,
      args: {},
      ctx: Context,
      info: GraphQLResolveInfo
    ) => string | Promise<string>;

    // more resolver definitions
  }

  export abstract class Class implements Type {
    id = (
      parent: User,
      args: {},
      ctx: Context,
      info: GraphQLResolveInfo
    ): string | Promise<string> => parent.id

    // more fields backed by `User` model ...
  }

}

In the case of a type-mismatch error, the current error messages aren't very helpful as seen below. In particular the message ... is not assignable to type 'Type' should be more concise by us picking a better name than Type.

[stephenh]

Naive suggestion, but just prefixes?

export const UserResolverDefaults = {
  id: (parent: User) => parent.id,
};

export type UserIdFieldResolver = (
  parent: User,
  args: {},
  ctx: Context,
  info: GraphQLResolveInfo
) => string | Promise<string>;
  
export interface UserResolver {
  id: UserIdFieldResolver;
}

[Weakky]

Update (breaking change)

We'll follow the same conventions as for flow, almost like you suggested Stephen, but with underscores to replace namespaces. eg:

export const User_defaultResolvers = {
  id: (parent: User) => parent.id,
};

export type User_Id_Resolver = (
  parent: User,
  args: {},
  ctx: Context,
  info: GraphQLResolveInfo
) => string | Promise<string>;
  
export interface User_Resolver {
  id: (
    parent: User,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => string | Promise<string>;
}

export interface Resolvers {
  User: User_Resolvers;
}

[mfix22]

Hi all, As an alternative approach to the type name prefix, here is another option.

We generate the following folder structure:

graphqlgen
├── index.ts
└── User.ts
└── ...

Where each GraphQL type is its own file that exports its Typescript type and resolver information. For example:

// Code generated by github.com/prisma/graphqlgen, DO NOT EDIT.

import { GraphQLResolveInfo } from 'graphql'
import { Context } from './context'

export const defaultResolvers = {}

export type VersionResolver = (
  parent: {},
  args: {},
  ctx: Context,
  info: GraphQLResolveInfo
) => string | null | Promise<string | null>

export type MyFieldResolver = (
  parent: {},
  args: {},
  ctx: Context,
  info: GraphQLResolveInfo
) => string | Promise<string>

export interface Type {
  version: (
    parent: {},
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => string | null | Promise<string | null>

  myField: (
    parent: {},
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => string | Promise<string>
}

and then imported in the resolvers as:

// Note the file is imported directly by type name
import { Type, defaultResolvers } from '../generated/graphqlgen/Query'

export const Query: Type = {
  ...defaultResolvers,
  version: parent => null
}

or

import * as QueryResolvers from '../generated/graphqlgen/Query'

export const Query: QueryResolvers.Type = {
  ...QueryResolvers.defaultResolvers,
  version: parent => null
}

As a follow up step (which can be done in a separate PR), I think we should just export the resolver map as the default export for resolver type files, which would simplify how the resolvers are imported:

import { Type, defaultResolvers } from '../generated/graphqlgen/Query'

const Query: Type = {
  ...defaultResolvers,
  version: parent => null,
  myField: parent => ''
}

export default Query

// and in resolvers/index.ts
import Query from './Query'

const resolvers: Resolvers = {
  Query
}

export default resolvers

Definitely open for feedback — but our team is available to create the PR for this 🙂

(cc @schickling)

[stephenh]

Peanut gallery: I really like the file-per-resolver output. Bonus points if I had schema/dir1/Foo.graphql schema/dir2/Bar.graphql and they ended up as generated/dir1/Foo.ts generated/dir2/Bar.ts, e.g. the folder structure of our input graphql files would be mirrored in the output resolver types.

[schickling]

This is a great suggestion @mfix22. I actually think that in combination with resolver scaffolding, splitting up the graphqlgen.ts file into multiple files makes sense and leads to cleaner generated and userland code.

@stephenh that's a great point. Let's consider #201 while looking into this.

[czystyl]

I use babel to transform the TS code. The current structure uses namespaces, but babel doesn't support them. So it is one more argument to move away from namespaces :)

@stephenh Do you need any help?

[stephenh]

FWIW bikeshedding names a bit, but even with the per-file outputs, I wouldn't mind/would generally prefer prefix-based type names.

E.g. for a given Foo graphql type, in its graphqlgen-generated foo.ts file, have a FooType (or FooResolverType, see below) type (which is the Type in the above example), export const fooDefaultResolvers for the defaults, etc.

The short/prefix-less names like Type are admittedly succinct, but I don't like ending up with ~100-200-500 some types all in the same project that are all called Type. When I use an IDE and open "find by type" then type Type, I'm going to get a ton of results. Same thing if I type export const FooResolver: Type<control-space> when I'm implementing the Foo resolver (to have the IDE auto-import it for me), I'm going to get a ton of suggestions. It is admittedly more chars, but export const FooResolver: Foo<control-space> will be much more helpful, in terms of auto-complete + find-by-type navigation.

Granted, on the import side of things import * as QueryResolvers from '../generated/graphqlgen/Query' is a potentially nice way to leverage * as ... as a qualifier, but IIRC most IDEs when they auto-complete the import are going to use the import { Type } from path version. And it doesn't help with the find-by-type navigation.

So, my $0.02, is to still prefer type-prefixed names; granted, this is biased by medium-to-large schemas which can have a different ROI of "few more chars here/there to get better tooling/IDE experience". And also personal preference.

(I am curious what the canonical names should be, e.g. we have both an implementation const, export const FooResolver = { // application code here } as well as a type FooResolver = { // type defintion here } declaration that can both somewhat claim to be "foo resolvers". So, even without the prefix/no-prefix preference, it's nice to have different names/terminology for those, e.g. (perhaps obviously) one is the resolver implementation vs. the other is the graphqlgen'd contract definition. Which leads me to thinking the resolver implementation should be const FooResolver = { ... } and the type contract is, well, a type, so (to avoid the IMO anti-pattern of using the same name for different things), it'd be type FooResolverType. Which leads to the boilerplate-but-canonical const FooResolver: FooResolverType = { ... } to get type-checking/inference to kick in. I admit it's not the sexiest, but its clear/consistent/explicit, which as you can tell from ^ is where I generally lean on these things. :-))

[schickling]

Thanks a lot for sharing your thoughts on this @stephenh. I completely see your points however I think in combination with a good scaffolding workflow this actually won't be an issue assuming you'd rarely write Type<control-space> yourself but mostly rely on the auto-scaffolded resolver files.

Here are two proposals:

1. Multi-file output

Output:

.
└── generated
    └── graphqlgen
        ├── Post.ts
        ├── User.ts
        └── index.ts

export const defaultResolvers = {
  id: (parent: User) => parent.id,
  // more fields backed by `User` model ...
};

export type id = (
  parent: User,
  args: {},
  ctx: Context,
  info: GraphQLResolveInfo
) => string | Promise<string>;

// more resolver types...

export interface Interface {
  id: (
    parent: User,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => string | Promise<string>;

  // more resolver definitions
}

export abstract class Class implements Interface {
  id = (
    parent: User,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ): string | Promise<string> => parent.id

  // more fields backed by `User` model ...
}

2. Prefixed output

Output:

.
└── generated
    └── graphqlgen.ts

export const User_defaultResolvers = {
  id: (parent: User) => parent.id,
  // more fields backed by `User` model ...
};

export type User_id = (
  parent: User,
  args: {},
  ctx: Context,
  info: GraphQLResolveInfo
) => string | Promise<string>;

// more resolver types...

export interface User_Interface {
  id: (
    parent: User,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => string | Promise<string>;

  // more resolver definitions
}

export abstract class User_Class implements User_Interface {
  id = (
    parent: User,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ): string | Promise<string> => parent.id

  // more fields backed by `User` model ...
}

Right now I'm leaning towards (1) but still open to go with (2) as well. Also note that I'm proposing to rename Type with Interface. With this change I'm rather focussing on the TypeScript/Flow terminology of interfaces vs classes instead of GraphQL types, enums etc.

[briandennis]

I'd toss my hat in for (1) as well if we're aiming for auto-scaffolded workflows 👍

[stephenh]

Having interfaces called Interface and classes called Class is weird to me, but for sure +1 for per-file either way.

E.g. not a huge fan of extends Class in:

import { Class } from '@src/generated/graphqlgen/User.ts`
class UserResolver extends Class {
   ...
}

Historically I've seen things like class UserResolver extends UserBaseResolver or class UserResolver extends UserResolverCodegen or class UserUser extends UserResolverDefaults.

But might just be repeating my same point but with a slightly different example, so np. :-)

Also nit-picky, but if generated/graphqlgen/User.ts doesn't have a User type in it, might name it user.ts to denote it's the collection of user types and not the file-with-the-User type (granted, kind of a grey area here).

[Weakky]

Update

After discussing with @schickling, here's the syntax we came up with (assuming we're splitting the graphqlgen.ts file):

Given the following GraphQL schema:

type User {
 ...
}

type Query {
  user(id: String!): User
}

The types for Query would be put into a Query.ts file and would contain the following types:

import { Context, User } from '../../types'
import { GraphQLResolveInfo } from 'graphql'

export const defaultResolvers = {}

export type ArgsUser = {
  id: string
}

export type UserResolver = (
  parent: never,
  args: ArgsUser,
  ctx: Context,
  info: GraphQLResolveInfo,
) => User | null | Promise<User | null>

export interface IQuery {
  user: (
    parent: never,
    args: ArgsUser,
    ctx: Context,
    info: GraphQLResolveInfo,
  ) => User | null | Promise<User | null>
}

export abstract class BaseQuery implements IQuery {
  abstract user: (
    parent: never,
    args: ArgsUser,
    ctx: Context,
    info: GraphQLResolveInfo,
  ) => User | null | Promise<User | null>
}

This would also fix #276

What do you think?

[briandennis]

@Weakky I'm fine with this format. To make sure I'm understanding it correctly, here's another example.

Given the GraphQL schema:

type Post {
  id: String!
  views: Int
  author: User!
}

type User {
  # ...
}

The types for Post would be put into a Post.ts file and would contain the following:

import { Context, User } from '../../types'
import { GraphQLResolveInfo } from 'graphql'

export const defaultResolvers = {
  id: (parent: Post) => parent.id,
  views: (parent: Post) => parent.views
};

export type IdResolver = (
    parent: Post,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => string | Promise<string>;

export type ViewsResolver = (
    parent: Post,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => number | null | Promise<number | null>;

export type AuthorResolver = (
    parent: Post,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => User | Promise<User>;

export interface IPost {
  id: (
    parent: Post,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => string | Promise<string>;

  views: (
    parent: Post,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => number | null | Promise<number | null>;

  author: (
    parent: Post,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => User | Promise<User>;
}

export abstract class BasePost implements IPost {
  id: (parent: Post) => parent.id
  views: (parent: Post) => parent.views
  
  abstract author: (
    parent: Post,
    args: {},
    ctx: Context,
    info: GraphQLResolveInfo
  ) => User | Promise<User>;
}

Does this all look correct? 🙂

[emmenko]

FYI: one unfortunate use case that I just encountered with namespacing is:

// Given this type (which is for a query predicate generated by prisma)
export type SomethingWhereInput = {
  +AND?: SomethingWhereInput[],
  +OR?: SomethingWhereInput[],
  +NOT?: SomethingWhereInput[],
  +id?: string,
  +id_not?: string,
  // ...
};

// after applying a namespace it results in
export interface FileNamespace_SomethingWhereInput {
  AND: SomethingWhereInput[];
  OR: SomethingWhereInput[];
  NOT: SomethingWhereInput[];
  id: string | null;
  id_not: string | null;
  // ...
};

Which causes flow to fail resolving the name SomethingWhereInput because it's been renamed to FileNamespace_SomethingWhereInput.

[schickling]

Absolutely correct @briandennis 💯 Would be great if you'd want to take a stab at this in a PR 🙌

[lgandecki]

Is there any update here? An alpha version that I can try to fix if it brakes for me? Or do we only have this written, rough "spec" that someone would have to start implementing from scratch?

[jfbrazeau]

Still no update ?