-
Notifications
You must be signed in to change notification settings - Fork 54
Rethink generated namespace/interface design for TypeScript #115
Comments
Naive suggestion, but just prefixes?
|
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;
} |
Hi all, We generate the following folder structure:
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) |
Peanut gallery: I really like the file-per-resolver output. Bonus points if I had |
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? |
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 The short/prefix-less names like Granted, on the import side of things 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, |
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 Here are two proposals: 1. Multi-file outputOutput:
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 outputOutput:
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. |
I'd toss my hat in for (1) as well if we're aiming for auto-scaffolded workflows 👍 |
Having interfaces called E.g. not a huge fan of
Historically I've seen things like But might just be repeating my same point but with a slightly different example, so np. :-) Also nit-picky, but if |
UpdateAfter discussing with @schickling, here's the syntax we came up with (assuming we're splitting the Given the following GraphQL schema: type User {
...
}
type Query {
user(id: String!): User
} The types for 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? |
@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 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? 🙂 |
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 |
Absolutely correct @briandennis 💯 Would be great if you'd want to take a stab at this in a PR 🙌 |
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? |
Still no update ? |
The current generated namespace/interface design for TypeScript for a
User
GraphQL type looks roughly the following: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 thanType
.The text was updated successfully, but these errors were encountered: