-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unsure how to do mutations for Class resolvers. #13
Comments
I found a workaround (but might not be pleasant).I am using Hono instead of Express by the way (so let me know if that's an issue). What I noticed is that there's a mismatch between the schema and resolvers considering that Grats is somewhat "schema-first"
❌1. What doesn't work (Using a Root Resolver and putting Query and Mutation properties on there and use it as a "root resolver")./** @gqlType */
class Root {
/** @gqlField */
query(): Query {
return new Query();
}
/** @gqlField */
mutation(): Mutation {
return new Mutation();
}
} app.use(
"/graphql",
graphqlServer({
schema,
rootResolver: (ctx) => new Root(),
})
);
{
query {
book
author
}
mutation {
updateBook
createBook
updateAuthor
}
}
{
book
author
updateBook
createBook
updateAuthor
} ❌2. What doesn't work # 2 (Spreading the
|
The way I've done this for https://api.webamp.org/graphql can be found here: Basically my query class extends my mutation class. This means it has all the properties of both types. I then pass that as my rootResolver. I suspect there's a better way here though, so I'll leave this open. |
Ohh right! Looking back, I think I should've just done that, extending the Mutation class. I think my solution of spreading the instances for every request seems less performant than your solution. Thanks for this. I'll keep playing around and see what else can work. Keep up the great work! 🎉🚀 |
So, I've confirmed that This does not fit very well with how Grats thinks about class-based resolvers. I'll continue thinking about a better way to model this in Grats. Maybe Grats could let you define something like: /** @gqlSchema */
export class Schema {
/** @gqlField */
query() {
return new Query({/* init options */});
}
/** @gqlField */
mutation() {
return new Mutation({/* init options */});
}
} |
my 2 cents: Having a /** @gqlType */
export default class MyCustomQuery { /* ... */ }
/** @gqlType */
export default class MyCustomMutation { /* ... */ }
/** @gqlSchema */
export class Schema {
/** @gqlField */
query() {
return new MyCustomQuery({/* init options */});
}
/** @gqlField */
mutation() {
return new MyCustomMutation({/* init options */});
}
} would become something like: type MyCustomQuery {
# ...
}
type MyCustomMutation {
# ...
}
schema {
query: MyCustomQuery
mutation: MyCustomMutation
} And it would also solve the same problem for subscriptions if Grats gets support for them. While it is an extra (wrapper) class, most of the time it will be small, not too much code, and (very important IMO) it eliminates ambiguity. I have no knowledge of Grats' internals, so what I don't know if this approach would force everyone to define a type Query {
# ...
} is the same as type Query {
# ...
}
schema {
query: Query
} Very exciting project btw, just started to play around with it (in Deno, after a bit of module remapping) |
I've looked into this a bit more, and from what I can tell, there's not a good way for Grats to be able to create the root object for a given operation and have it get passed to the top level field resolvers. Specifically, there's not really any place to hook in and create the object once per request. TypeGraphQL seems to lean into this and treats these top level objects as global:
-- https://typegraphql.com/docs/resolvers.html I'm skeptical of this approach, since data structures that are shared between requests feels like a recipe for leaking private data between users unless one is very careful. I'd prefer that dependency injection be done by adding data to the request context. Looking forwardI'm still exploring options here. For example:
|
Regarding Deno, I'm curious to hear about your experience! Are there things Grats could do better that would make it work more smoothly? If so, maybe you could open an issue and we could discuss there? |
Just sharing another workaround I found, pulling stuff from the "mixins" pattern from the Typescript docs and a StackOverflow answer: /**
* The type of whatever arg we want to pass to the resolvers' constructor
*/
type ResolverConstructorParams = typeof ctx;
/**
* @gqlType Query
*/
class QueryResolver {
constructor(
init: ResolverConstructorParams,
// we could add a dataloader here.
// dataloader: DataLoader,
) {
/* ... */
}
/* ... */
}
/**
* @gqlType Mutation
*/
class MutationResolver {
// constructor signature has to match the rest if we want to combine them together
constructor(
init: ResolverConstructorParams,
// If we added a dataloader to QueryResolver's constructor,
// we would need to add it here as well for the signatures to match.
// The constructor's body can ignore it if it's not used.
// dataloader: DataLoader,
) {
/* ... */
}
/* ... */
}
/*
* The type hacks begin here
*/
type ResolverConstructor<T, Args extends unknown[]> = new (...args: Args) => T;
function Classes<Args extends unknown[]>(...bases: ResolverConstructor<unknown, Args>[]) {
class Bases {
constructor(...args: Args) {
for (const base of bases) {
Object.assign(this, new base(...args))
}
}
}
for (const base of bases) {
for (const propName of Object.getOwnPropertyNames(base.prototype).filter(prop => prop != 'constructor')) {
if (Object.prototype.hasOwnProperty.call(Bases.prototype, propName)) {
throw new TypeError(`"${propName}" has already been defined in ${Bases.name}`);
}
Object.defineProperty(
Bases.prototype,
propName,
Object.getOwnPropertyDescriptor(base.prototype, propName) ||
Object.create(null)
);
}
}
return Bases;
}
/*
* End of type hacks
*/
// We need both the `class` and `interface` definitions
export interface RootResolver extends QueryResolver, MutationResolver {}
export class RootResolver extends Classes(QueryResolver, MutationResolver) {}
// server.ts
// We now use a new instance of `RootResolver` to handle requests
app.use(
"/graphql",
graphqlServer({
schema,
rootResolver: ctx => new RootResolver(ctx),
})
); This is a very hacky attempt to emulate multiple inheritance, but at least the hacky looking stuff is well contained. This would also allow passing params to the resolvers' constructors, and will force all of these to have the same signature. It probably contains bugs and limitations (for example, all instance properties must be enumerable for all of this to work) |
@mandx Thanks for sharing that workaround. I can see that working! However, looking around at the ecosystem of GraphQL servers, there does not seem to be a standard way to provide a root value on a per-request basis. This is leaning me toward simply disallowing root objects all together since there's no standard way to implement it that would work with all GraphQL server implementations. Even when it is possible, how frequently the root values get recreated (once per request? once for the lifetime of the app?) is not standardized. May I ask what you are currently using your root value for? For example, what do you use I'll try to take one more deep read through GraphQLSchema to see if I can see some way for us to hack this in before I give up on root values all together and head in the direction of all root fields being state-less (parent-less). |
Strictly speaking, we already have a mechanism to provide a "context" object to resolvers, and that's in the My main pet-peeve with However, I realize that to get this working I had to reach for some weird type acrobatics that I probably don't 100% understand, doesn't look that elegant and none of this is applicable to pure-function-based resolvers. The key part is to provide a way to create dynamic context values, and IMO this falls out of the scope of Grats; that's on the "glue code" that connects a raw request handler with a GraphQL executor. With Deno's |
I agree that the typing of context values is generally awkward. Since there are no static calls to resolver functions, there is no type-safety there. However, I think Grats is well positioned to provide this validation. We could provide some mechanism for your grats config to indicate which type you use as your context value, and Grats could validate a build time that your types conform. It can't ensure you actually provide that value to your executor, but that's defined in a single place. I feel like this is a more robust solution to the context problem, and has well defined semantics (context values are tied to the lifetime of the request). |
I've filed #21 to track the idea of allowing Grats to validate context values. |
Ooohh I see what you mean now. Continuing the "context"-related discussion there. |
Looking at the GraphQL docs again and this sentence stood out where it describes the parent object argument to resolvers (emphasis mine).
https://graphql.org/learn/execution/#root-fields-resolvers I think this supports my intuition that we should just enforce that the |
Seems to me that (at least for now) this is the way to go. In the future we could consider add plain objects, like: /** @gqlType */
export const Query = {
/** @gqlField */
me(): string {
return '';
}
}; // maybe this would need `as const`? Problem is, there's still a missing piece, and is the utility (that IMO should be provided by Grats) that would inspect the Grats-generated schema, "collect" all these functions resolvers (everything that act on Here's how it could look like: import {
buildSchemaFromSDL,
extractGratsSchemaAtRuntime,
buildRootResolverFromGratsSchema, // -> New API (placeholder name)
} from 'grats';
const schema = process.env.FROM_SDL
? buildSchemaFromSDL(readFileSync("./schema.graphql", "utf8"))
: extractGratsSchemaAtRuntime({ emitSchemaFile: "./schema.graphql" });
/**
* Since (I'm assuming) the schema has encoded all the locations of
* the resolvers functions/objects/etc. this would create a new object
* with all the top level stuff merged into it, plus some additional
* checks performed as necessary.
*/
const rootResolver = buildRootResolverFromGratsSchema(schema);
app.post(
"/graphql",
createHandler({
schema: schema,
rootValue: rootResolver,
}),
); For instance: /**
* @gqlField
*/
type Query {}
/**
* @gqlField
*/
function me(_: Query): string {
return 'me';
}
/**
* @gqlField
*/
type Mutation {}
/**
* @gqlField
*/
function sayHi(_: Mutation, { name } { name: string }): string {
return 'hi!';
}
const resolver = buildRootResolverFromGratsSchema(schema);
/*
typeof resolver === {
me: () => string;
sayHi: (name: string) => string;
}
*/ This |
Another idea (pulling from the context discussion we had elsewhere). We could enforce that all of your operation types were aliases of the same type: type RootType = {
db: DBHandle
};
/** @gqlType */
type Query = RootType;
/** @gqlType */
type Mutation = RootType;
/** @gqlField */
export function me(root: Query): User {
return new User(db.getUserById("4"));
}
/** @gqlField */
export function deleteMe(root: Mutation): boolean {
return db.deleteUserById("4");
} Just like context we could then either require a Then it would be up to the user to configure their executor to provide a root value of the type they specified. Again, this is just like how context works. |
But don't we still need something that "collects" all the top-level operation fields into something to me used for the |
In my example above we would expect the user to pass an object of type In Yoga I don't think you get to pass any value, so you'd need to use |
wouldn't {
me: (root: Query) => User;
deleteMe: (root: Mutation) => boolean;
} for resolution to work properly? Yoga is a bit different, but it still expects a |
If you define a field using a free function, Grats will monkey patch the field type so that resolver is replaced with a call to the function. So, if all your fields are defined using free functions, the individual field methods don't need to be attached to the rootValue object. |
Ooh I see, very cool. This makes me lean some more towards the idea of making free functions the preferred (if not the only) resolver style for top level fields. |
Is there a recommended way? I can't find anything in the docs.
I tried making a Root class and used that as the root resolver.
The Root class contains a Mutation and a Query class.
Except this doesn't work.
The example express app you have only uses Query.
The text was updated successfully, but these errors were encountered: