eddeee888 / graphql-code-generator-plugins

List of GraphQL Code Generator plugins that complements the official plugins.
MIT License
44 stars 10 forks source link

[Q] generic node lookup by id should provide a \"resolveType\" function or each possible type should provide an \"isTypeOf\" function #289

Closed marceloverdijk closed 2 weeks ago

marceloverdijk commented 1 month ago

My schema has a general (Relay) lookup query like:

Query {
  node(id: ID!): Node
}

The code generated for this is:

import type { QueryResolvers } from '../types.generated';

export const node: NonNullable<QueryResolvers['node']> = async (_parent, args, ctx) => {
};

which I'm trying to implement like:

import type { QueryResolvers } from '../types.generated';
import { GraphQLError } from 'graphql';
import { parseGlobalId } from '@/api/globalid';

export const node: NonNullable<QueryResolvers['node']> = async (_parent, args, ctx) => {
  const { type } = parseGlobalId(args.id);
  switch (type) {
    case 'Circuit':
      const { parts: id } = parseGlobalId<string>(args.id);
      return ctx.circuitService.findById(id);
    // TODO: other types.
    default:
      throw new GraphQLError('Invalid ID.');
  }
};

but when trying it out with a query like:

query MyQuery {
  node(id: "Q2lyY3VpdDphZGVsYWlkZQ==") {
    ... on Circuit {
      id
      name
    }
  }
}

it gives the following error:

{
  "errors": [
    {
      "message": "Abstract type \"Node\" must resolve to an Object type at runtime for field \"Query.node\". Either the \"Node\" type should provide a \"resolveType\" function or each possible type should provide an \"isTypeOf\" function.",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "node"
      ]
    }
  ],
  "data": {
    "node": null
  }
}

Should I do something else? Provide a \"resolveType\" function or provide an \"isTypeOf\" function? But how to do that?

eddeee888 commented 1 month ago

Hi @marceloverdijk !

There are 3 ways to handle an abstract type in GraphQL i.e. Interface and Union:

  1. Use __resolveType in the abstract type
  2. Use __isTypeOf in the implementing type for interface or member type for union
  3. Use __typename in the resolver that returns the node

Let's extend your schema with a few types:

interface Node {
  id: ID!
}

type Circuit implements Node {
  id: ID!
}

type Track implements Node {
  id: ID!
}

type Foo implements Node {
  id: ID!
}

type Bar implements Node {
  id: ID!
}

1. Using __resolveType

Here's an example:

import type { NodeResolvers } from "./../../types.generated";
export const Node: NodeResolvers = {
  __resolveType: (parent) => {
    switch (obj.type) {
      case 'Circuit':
        return 'Circuit';
      case 'Track':
        return 'Track';
      case 'Foo':
        return 'Foo';
      case 'Bar':
        return 'Bar';
      default:
        throw new GraphQLError('Invalid Node type');
    }
  }
};

This way, we are assuming each object has a type in its mapper, so that we can return the mapper into resolvers that expect a Node e.g.

type CircuitMapper = { id: string, type: 'Circuit' };
type TrackMapper = { id: string, type: 'Track' };
// ... and so on

2. Using __isTypeOf

With this approach, we just add __isTypeOf to every implementing types e.g.

export const Circuit: CircuitResolvers = {
  __isTypeOf: (parent) => parent.type === "Circuit"
}

export const Track: TrackResolvers = {
  __isTypeOf: (parent) => parent.type === "Track"
}

Similar to option 1, we need to create mappers. As you may notice, we move the switch paths to every type that implements Node

3. Using __typename

We can return __typename in Query.node, declaring what type it is

export const node: NonNullable<QueryResolvers['node']> = async (_parent, args, ctx) => {
  const { type } = parseGlobalId(args.id);
  switch (type) {
    case 'Circuit':
      const { parts: id } = parseGlobalId<string>(args.id);
      return {
        __typename: 'Circuit', // declaring here means we don't have to do it in other places
        ...ctx.circuitService.findById(id)
      };
    // TODO: other types.
    default:
      throw new GraphQLError('Invalid ID.');
  }
};

Note that this is the default for Server Preset. This keeps the logic all in one place, making it a bit easier to understand (in my opinion at least)


For your case, I think there's some custom config in your codegen that turns off required typename for interfaces and union nodes? If this is your preference, you'd have to implement them in interfaces using __resolveType or implementing object types using __isTypeOf

marceloverdijk commented 4 weeks ago

Thank you @eddeee888 I appreciate your response; I was missing the __typename: 'Circuit' as you explained in option 3.

To follow up, I'm now using this:

export const node: NonNullable<QueryResolvers['node']> = async (_parent, args, ctx) => {
  const { type, parts } = parseGlobalId(args.id);
  switch (type) {
    case 'Circuit': {
      const id = parts as string;
      return {
        __typename: 'Circuit',
        ...(await ctx.circuitService.findById(id)),
      };
    }
    case 'Driver': {
      const id = parts as string;
      return {
        __typename: 'Driver',
        ...(await ctx.driverService.findById(id)),
      };
    }
    case 'Season': {
      const year = parts as number;
      return {
        __typename: 'Season',
        ...(await ctx.seasonService.findByYear(year)),
      };
    }
    case 'Race': {
      const [year, round] = parts as [number, number];
      return {
        __typename: 'Race',
        ...(await ctx.raceService.findByYearAndRound(year, round)),
      };
    }
    // TODO: other types.
    default:
      throw new GraphQLError('Invalid ID.');
  }
};

but that does not yet handle that a node cannot be found, e.g. a id requested that does not exists anymore.

So probably best to check for that and return null if not found?

export const node: NonNullable<QueryResolvers['node']> = async (_parent, args, ctx) => {
  const { type, parts } = parseGlobalId(args.id);
  switch (type) {
    case 'Circuit': {
      const id = parts as string;
      const circuit = await ctx.circuitService.findById(id);
      if (!circuit) {
        return null;
      }
      return {
        __typename: 'Circuit',
        ...circuit,
      };
    }

Thinking about it, would I be able to call the actual circuit resolver instead that I also have to reuse that code? You see what I mean below?

Query {
  node(id: ID!): Node
  circuit(id: ID!): Circuit
}

--

import type { QueryResolvers } from '../types.generated';
import { parseGlobalId } from '@/api/globalid';

export const circuit: NonNullable<QueryResolvers['circuit']> = async (_parent, args, ctx) => {
  const { parts: id } = parseGlobalId<string>(args.id, 'Circuit');
  return ctx.circuitService.findById(id);
};

PS: this is my config:

import type { CodegenConfig } from '@graphql-codegen/cli';
import { defineConfig } from '@eddeee888/gcg-typescript-resolver-files';

const config: CodegenConfig = {
  schema: './src/api/graphql/schema.graphql',
  generates: {
    'src/api/graphql/impl': defineConfig({
      mode: 'merged',
      typesPluginsConfig: {
        contextType: '../context#Context',
      },
    }),
  },
  hooks: {
    afterAllFileWrite: ['prettier --write'],
  },
};

export default config;
eddeee888 commented 4 weeks ago

Hi @marceloverdijk , that's right for both points πŸ™‚

  1. You'll need to handle the null case similar to what you have
  2. If you already know that the id you have in the client is for a Circuit, it'd be simpler just to call Query.circuit. the general Query.node looks like it's useful if we don't know which object the id is for πŸ™‚

Note that I personally find that Query.node may become a "god resolver" where it must know how to resolve every type that implements it (e.g. if we are planning to use it for every domain object type in the Graph)

marceloverdijk commented 4 weeks ago

It's indeed some "god resolver" like you said (implemented as required by Relay).

It would be nice to do something like this:

export const node: NonNullable<QueryResolvers['node']> = async (_parent, args, ctx) => {
  const { type, parts } = parseGlobalId(args.id);
  switch (type) {
    case 'Circuit': {
      // call the Query.circuit resolver directly so I have that logic in just 1 place.
    }
    ..

Wdyt?

eddeee888 commented 3 weeks ago

Ah, interesting, I've seen that in places, now I know it's a Relay pattern.

In this case, we can call the Query.circuit resolver directly, however, each resolver has a 4th param is generated automatically by GraphQL:

import { circuit } from './circuit';

export const node: NonNullable<QueryResolvers['node']> = async (_parent, args, ctx) => {
  const { type, parts } = parseGlobalId(args.id);
  switch (type) {
    case 'Circuit': {
      circuit(null, { id: args.id }, ctx, { /* ... info param */ })
    }

This param has certain values that is related to the client query e.g. path contains the current resolver path, etc. So it'd be hard to create it manually in Query.node I think.

However, maybe we could abstract just the main logic (maybe include error handling too?) from theses queries? e.g.

const fetchCircuit = (id: string): Promise<Circuit> => { ... }

// Query.node
case 'Circuit': {
  fetchCircuit(id)
}

// Query.circuit
const circuit = (parent, {id}) => fetchCircuit(id)
marceloverdijk commented 3 weeks ago

Yes πŸ‘ , this is what I did for now:

circuit.ts:


import type { QueryResolvers } from '../types.generated';
import type { Context } from '@/api/graphql/context';
import { parseGlobalId } from '@/api/globalid';
import { type Circuit } from '@/drizzle/f1db';

export const circuit: NonNullable<QueryResolvers['circuit']> = async (_parent, args, ctx) => { return fetchCircuit(args, ctx); };

export const fetchCircuit = async (args: { id: string }, ctx: Context): Promise<(Circuit & { typename: 'Circuit' }) | null> => { const { parts: id } = parseGlobalId(args.id, 'Circuit'); const circuit = await ctx.circuitService.findById(id); if (!circuit) { return null; } return { typename: 'Circuit', ...circuit }; };


> node.ts:

import type { QueryResolvers } from '../types.generated'; import { fetchCircuit } from './circuit'; import { GraphQLError } from 'graphql'; import { parseGlobalId } from '@/api/globalid';

export const node: NonNullable<QueryResolvers['node']> = async (_parent, args, ctx) => { const { type, parts } = parseGlobalId(args.id); switch (type) { case 'Circuit': { return fetchCircuit(args, ctx); } .. other cases + default handling



Note I had to add the `& { __typename: 'Circuit' }` part to `fetchCircuit` to make it typesafe.
marceloverdijk commented 3 weeks ago

and I think this is probably even better:

export const fetchCircuit = async (args: RequireFields<QuerycircuitArgs, "id">, ctx: Context): Promise<(Circuit & { __typename: 'Circuit' }) | null> => {
  ..
}
eddeee888 commented 3 weeks ago

Yes, I think that's a good option πŸ‘ However, this option is returning __typename even when we don't need it in Query.cirtcuit.

Alternatively, the option where we handle null check in Query.node is also viable: it doesn't make the assumption where it's used (even if it's a bit more verbose)

Both options are doesn't quite fit the current use case well unfortunately. πŸ€”


Alternatively, maybe fetchCircuit can wrap __typename handling with function overloads? e.g.

function fetchCircuit (args: RequireFields<QuerycircuitArgs, "id">, ctx: Context, typename: null): Promise<Circuit | null>
function fetchCircuit (args: RequireFields<QuerycircuitArgs, "id">, ctx: Context, typename: 'Circuit'): Promise< Circuit & { __typename: 'Circuit' } >
function fetchCircuit (args: RequireFields<QuerycircuitArgs, "id">, ctx: Context, typename: null | 'Circuit'): Promise<(Circuit & { __typename: 'Circuit' }) | null> |  Promise<Circuit | null> => {
  if(typename) {
    return /* fetch proomise*/
  }

  const result = await /* fetch function */

  if(result === null) {
    return null;
  }

  return {
    __typename: 'Circuit',
    ...result,
  }
}

However, after typing this out, it just moves the null check to fetchCircuit which makes it a bit harder to read. So maybe not great compared to the other options you already mentioned. πŸ™‚

eddeee888 commented 2 weeks ago

Thanks for the discussion @marceloverdijk ! I'll close this issue if there's no more questions.