Skip to content
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

Closed
Blankeos opened this issue Oct 14, 2023 · 22 comments · Fixed by #62
Closed

Unsure how to do mutations for Class resolvers. #13

Blankeos opened this issue Oct 14, 2023 · 22 comments · Fixed by #62

Comments

@Blankeos
Copy link

Blankeos commented Oct 14, 2023

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.

@Blankeos
Copy link
Author

Blankeos commented Oct 14, 2023

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"

"Schema-first" in the sense that you can tsdoc all the schema types anywhere you want, but it's still up to you to make sure that the "Root Resolver" you pass in the graphQL server is honors this schema you wrote).

❌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(),
  })
);

This doesn't work because the once you define Mutation and Query types in your schema, they automatically become the the topmost part of the schema. Root here is really just considered another "type" that isn't even under Query. It's still not considered topmost.

Also, I found that the queries and mutations actually don't have to be under "query" and "mutation" objects.

{
   query {
      book
      author
   }
   mutation {
      updateBook
      createBook
      updateAuthor
   }
}

In fact, they're supposed to be like this: (in one object)!

{
   book
   author
   updateBook
   createBook
   updateAuthor
}

❌2. What doesn't work # 2 (Spreading the new Query() and new Mutation() classes on the rootResolver).

Knowing that when you have to define your resolvers in a way that Queries and Mutations should be on the same level, not under a field of query { } or mutation { }, then spreading the two "instance of classes" would work right? Here's what I did:

/** @gqlType */
export class Query {
...
}

export class Mutation{
...
}
app.use(
  "/graphql",
  graphqlServer({
    schema,
    rootResolver: (ctx) => ({
      ...new Query(),
      ...new Mutation(),
    }),
  })
);

Except this doesn't work because "instance of classes"(supposedly Objects), don't actually work the same way "objects (POJOs)" in JavaScript. If you're interested why, look up "enumerable properties" vs "non-enumerable properties" in Javascript.

TL;DR of that is, even if you spread as the example above, those properties and methods of the object actually don't exist under the "instance of the class" (object) so they won't get spreaded because they're "non-enumerable properties". SPREAD and ASSIGN only deal with "enumerable properties".

✅ 3. Best work-around the works for me so far is this:

There is a way to make them "enumerable" or exist within the "instance". Using a constructor and assigning them with this. The reason I don't like this is that for every Query I make, I'll have to make sure I'm assigning them to the "instance" with this. That's the cleanest work-around I could find so far. Although as far as I know, I only need to do this for the Query and Mutation classes. 👍

/** @gqlType */
export class Query {
  constructor() {
    this.hello = this.hello;
    this.test = this.test;
    this.wow = this.wow;
    this.book = this.book;
    this.author = this.author;
    this.books = this.books;
  }
  
  
  /** @gqlField */
  hello(): string {
    return "Hello world!";
  }
  
  ...
}

/** @gqlType */
export class Mutation {
  constructor() {
    this.updateBook = this.updateBook;
  }

  /** @gqlField */
  updateBook(args: { id: string; newTitle: string }): Book {
    const book = db.books.find((book) => book.id === args.id);
    if (!book) throw Error("Could not find book.");
    book.title = args.newTitle;

    return new Book(args.id);
  }
  
  ...
}
app.use(
  "/graphql",
  graphqlServer({
    schema,
    rootResolver: (ctx) => ({
      ...new Query(),
      ...new Mutation(),
    }),
  })
);

Let me know how you guys would approach this. Happy coding!

@captbaritone
Copy link
Owner

The way I've done this for https://api.webamp.org/graphql can be found here:

https://github.com/captbaritone/webamp/blob/25b5579157c1a5e1539ffdb68e434afdf809bbe6/packages/skin-database/api/graphql/resolvers/RootResolver.ts#L29

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.

@Blankeos
Copy link
Author

Blankeos commented Oct 15, 2023

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! 🎉🚀

@captbaritone
Copy link
Owner

So, I've confirmed that graphql-js's execute assumes that you have a shared rootValue for both Mutation and Query: https://github.com/graphql/graphql-js/blob/7a6d055defd7a888f50c6133faae2b9010d45fad/src/execution/execute.ts#L432-L443

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 */});
  }
}

@mandx
Copy link
Contributor

mandx commented Oct 27, 2023

my 2 cents: Having a Schema class (or function?) mirrors what's possible already with GraphQL SDL. Extending on @captbaritone 's example:

/** @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 Schema class, or just using a Query as a resolver could be used as a shortcut, much like GraphQL SDL, which allows omitting the schema declaration, if the query type is literally named Query. In other words, this SDL:

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)

@captbaritone
Copy link
Owner

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:

We can use a DI framework (as described in the dependency injection docs) to inject class dependencies (like services or repositories) or to store data inside the resolver class - it's guaranteed to be a single instance per app.

-- 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 forward

I'm still exploring options here. For example:

  1. You define top level fields by defining a function that takes no root object but is annotated with something like @gqlQueryField
  2. You define your Query and Mutation types using the existing @gqlType annotation, but we enforce that the actual type must be undefined: type Query = undefined.
  3. Grats itself exports pre-existing Query/Mutation/Subscription types which you can use (but are typed as undefiend, so you don't get access to any concrete value)

@captbaritone
Copy link
Owner

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?

@mandx
Copy link
Contributor

mandx commented Oct 28, 2023

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?

I have opened an issue and created a reproduction repo with my findings so far.

@mandx
Copy link
Contributor

mandx commented Oct 30, 2023

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)

@captbaritone
Copy link
Owner

@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. express-graphql (deprecated) uses the same object for all requests and Yoga does not seem to provide a way to create one at all. Ideally Grats could arrange to initialize the root value(s) once per request, but I don't see a way to do that within GraphQLSchema, and that's currently the structure Grats returns.

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 ctx for in the example you provide?

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).

@mandx
Copy link
Contributor

mandx commented Nov 2, 2023

May I ask what you are currently using your root value for? For example, what do you use ctx for in the example you provide?

Strictly speaking, we already have a mechanism to provide a "context" object to resolvers, and that's in the contextValue key (right next to rootValue) in the object passed to pretty much any graphql-js based handler, which in turn, as you know, it's passed as the third argument to a resolver method/function. Setting contextValue or passing that to a contructor would be two ways of having shared data/resources available to all resolvers within that request.

My main pet-peeve with contextValue is that is typed as any/unknown, so when writing the resolver method, we have to specific the type we want, and trust that the value will be of that type. Using constructor constructor arguments on the other hand, provides better type safety (IMO).

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 gql is simple because we are handle the request directly; with Yoga, one would provide a factory function as contextValue and same with graphql-http (which is supposed to be the successor to express-graphql).

@captbaritone
Copy link
Owner

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).

@captbaritone
Copy link
Owner

I've filed #21 to track the idea of allowing Grats to validate context values.

@mandx
Copy link
Contributor

mandx commented Nov 2, 2023

Ooohh I see what you mean now. Continuing the "context"-related discussion there.

@captbaritone
Copy link
Owner

Looking at the GraphQL docs again and this sentence stood out where it describes the parent object argument to resolvers (emphasis mine).

  • obj The previous object, which for a field on the root Query type is often not used.

https://graphql.org/learn/execution/#root-fields-resolvers

I think this supports my intuition that we should just enforce that the Query/Mutation/Subscription types be empty in some way and use function resolvers for top-level fields.

@mandx
Copy link
Contributor

mandx commented Nov 6, 2023

I think this supports my intuition that we should just enforce that the Query/Mutation/Subscription types be empty in some way and use function resolvers for top-level fields.

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 schema.query/schema.mutation/schema.subscription/etc and build a new object with all of these as methods/fields.

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 buildRootResolverFromGratsSchema API would also perform any checks necessary, and, same as buildSchemaFromSDL and extractGratsSchemaAtRuntime APIs, it would be expected to be run once during startup, not during handling of individual requests/operations.

@captbaritone
Copy link
Owner

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 /** @gqlRootType */ on that root type, or just enforce that they all match.

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.

@mandx
Copy link
Contributor

mandx commented Nov 8, 2023

But don't we still need something that "collects" all the top-level operation fields into something to me used for the rootValue in graphql-js based frameworks?

@captbaritone
Copy link
Owner

In my example above we would expect the user to pass an object of type RootType as their rootValue. This same object would be passed as the "parent object" to each top level field (mutation, subscription or query). This matches the behavior of graphql-http and others.

In Yoga I don't think you get to pass any value, so you'd need to use undefined or unknown as your RootValue type.

@mandx
Copy link
Contributor

mandx commented Nov 8, 2023

wouldn't rootValue require an object having a type like:

{
  me: (root: Query) => User;
  deleteMe: (root: Mutation) => boolean;
}

for resolution to work properly?

Yoga is a bit different, but it still expects a resolvers key for the same purpose.

@captbaritone
Copy link
Owner

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.

@mandx
Copy link
Contributor

mandx commented Nov 12, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants