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

Custom Default Resolver #733

Closed
southpolesteve opened this issue Feb 28, 2017 · 8 comments
Closed

Custom Default Resolver #733

southpolesteve opened this issue Feb 28, 2017 · 8 comments

Comments

@southpolesteve
Copy link

We are finding ourselves writing a lot of field resolvers that have very similar logic. There are a number of ways to deal with this, but I would like to propose adding a defaultResolve option that will override the built in default resolver for any fields of an object.

Before

const Post = new GraphQLObjectType({
  name: 'Post',
  fields: {
    title: {
      type: GraphQLString,
      resolve: (root, args, ctx) => ctx.db.load(root, 'title')
    },
    author: {
      type: GraphQLString,
      resolve: (root, args, ctx) => ctx.db.load(root, 'author')
    }
  }
})

After

const Post = new GraphQLObjectType({
  name: 'Post',
  defaultResolve: (root, args, ctx, info) => ctx.db.load(root, info.fieldName)
  fields: {
    title: { type: GraphQLString },
    author: { type: GraphQLString }
  }
})

You can imagine how much this dries up code when you have a lot of fields. I am happy to supply a PR to implement this if the maintainers feel it would be a valuable addition.

@stubailo
Copy link

stubailo commented Mar 1, 2017

Would also be useful to have that for the whole schema, not just individual types!

@southpolesteve
Copy link
Author

PR: #734

The general idea is working but I need to add some docs and probably tweak a bit to meet the repo coding style.

@southpolesteve
Copy link
Author

@leebyron any thoughts on this issue? I have a PR submitted that implements this. Happy to go in a different direction if you have other ideas.

@leebyron
Copy link
Contributor

Thanks for investigating this @southpolesteve! I don't think Objects should have resolver functions as it introduces ambiguity. That resolver would be used for every Field on that Object, but at first I misread it as the return type Object's resolver that would be called instead of the parent type. I wouldn't be surprised if this was a common point of confusion.

I agree with @stubailo's suggestion that a whole-schema resolver would be more generally useful. If provided as an argument to execute() it could be defined and provided independently of the schema.

@southpolesteve
Copy link
Author

@leebyron What about changing the name to something clearer like defaultFieldResolver? At Bustle, we use graphQL to rollup disparate services and databases. Controlling it at the object level is really the ideal place for us. 4 types might share a default resolver but that same resolver won't work schema wide.

In the mean time we have worked around this by extending the built in types but it feels a bit hacky to me. Here is a somewhat contrived example but nearly identical in structure to actual code we have in production:

export class GraphQLRedisType extends GraphQLObjectType {
  constructor (config) {
    const oldFields = config.fields
    config.interfaces = config.interfaces || []
    config.interfaces.push(Redis)
    config.fields = () => {
      const fields = typeof oldFields === 'function' ? oldFields() : oldFields
      Object.keys(fields).map((fieldName) => {
        if (!fields[fieldName].resolve) {
          fields[fieldName].resolve = redisFieldResolver
        }
      })

      return { ...redisSharedFields, ...fields }
    }
    super(config)
  }
}

I can see why having a schema level default would be a nice thing but it doesn't really address why I went down this path. I can build that into another PR.

@leebyron
Copy link
Contributor

I actually really like that pattern of type builders that incorporate platform custom behavior. I don't think it's hacky at all.

I'd rather not include a resolver for a type because of the ambiguity and confusion I mentioned in my previous comment. Separate people who have brought up this idea have all wanted it to mean something different, and that's definitely a red flag for semantics.

Closing this issue - but will pursue the separate but related issue of a global default resolver in a separate issue

leebyron added a commit that referenced this issue May 18, 2017
This adds an argument to `execute()`, `createSourceEventStream()` and `subscribe()` which allows for providing a custom field resolver. For subscriptions, this allows for externalizing the resolving of event streams to a separate function (mentioned in #846) or to provide a custom function for externalizing the resolving of values (mentioned in #733)

cc @stubailo @helfer
leebyron added a commit that referenced this issue May 18, 2017
This adds an argument to `execute()`, `createSourceEventStream()` and `subscribe()` which allows for providing a custom field resolver. For subscriptions, this allows for externalizing the resolving of event streams to a separate function (mentioned in #846) or to provide a custom function for externalizing the resolving of values (mentioned in #733)

cc @stubailo @helfer
leebyron added a commit that referenced this issue May 18, 2017
This adds an argument to `execute()`, `createSourceEventStream()` and `subscribe()` which allows for providing a custom field resolver. For subscriptions, this allows for externalizing the resolving of event streams to a separate function (mentioned in #846) or to provide a custom function for externalizing the resolving of values (mentioned in #733)

cc @stubailo @helfer
@timhuff
Copy link

timhuff commented Feb 12, 2018

I was helped by this page so I figured I'd share alike. Inspired by @southpolesteve, I wrote the follow. It's a little different.

One thing that differs is that it doesn't assume a resolve method. It instead creates two Schema templates - one for a normal schema and one for a list schema. If you decide to use these, then it's assumed that you've defined the appropriate methods on that subclass (and you'll get an error if you haven't, rather than the thing just silently failing).

Another difference is that I decided to take the opportunity to resolve the circular dependency problem. graphql permits you to put fields as a function that's deferred a tick but that really only solves the problem if you have all these things defined in the same file. I feel that the empty parameter list when calling that function is a real missed opportunity, so I pass a hash of the created types (accumulated in the types object) into the fields function when I call it. This permits the subclass to extract them for usage in its fields definition (see SessionType below).

I later decided to replace types with a Proxy (see bottom). In the event that a type is removed and you forget that some other subclass is dependent upon it, it will throw an error informing you of this fact. I left this out of the code below in an effort to keep it simple (to include, just replace let types = {} with the code snippet at the bottom)

Footnote:

I commented out a line that makes reference to gs. This is the package graphql-sequelize, which offers utils for defining graphql schemas in terms of sequelize models.

Base Class

import { GraphQLObjectType, GraphQLList } from 'graphql'

let types = {}
export default class ModelType extends GraphQLObjectType {
  constructor({ fields, ...config }) {
    super({
      ...config,
      fields: typeof fields === 'function' ? () => fields(types) : fields,
    })

    types[this.constructor.name] = this

    this.Schema = {
      type: this,
      resolve: this.get,
    }
    this.ListSchema = {
      type: new GraphQLList(this),
      resolve: this.query,
    }

    this.get = this.get.bind(this)
    this.query = this.query.bind(this)
  }

  get() {
    throw new Error(
      `Someone is trying to use ${this.constructor.name}.Schema but no "get" function is defined for that type.`
    )
  }
  query() {
    throw new Error(
      `Someone is trying to use ${this.constructor.name}.ListSchema but no "query" function is defined for that type.`
    )
  }
}

Extended Class:

class SessionType extends ModelType {
  constructor() {
    super({
      name: 'Session',
      description: "A user's web session",
      fields: ({ UserType }) => ({
        // ...gs.attributeFields(DB.Session, { exclude: ['token'] }),
        user: {
          ...UserType.Schema,
          resolve: this.getUser,
        },
      }),
    })
  }

  async get(parent, args, context) {
    // ...
  }

  async getUser(parent, args, context) {
    // ...
  }

  async create(parent, { email, password }) {
    // ...
  }
}

Schema definition

import { SessionType } from 'api/types'

export default new GraphQLSchema({
  query: new GraphQLObjectType({
    name: 'Queries',
    description: 'Root query object',
    fields() {
      return {
        session: SessionType.Schema,
      }
    },
  }),
  mutation: new GraphQLObjectType({
    name: 'Mutations',
    description: 'Functions to create or modify stuff',
    fields() {
      return {
        create_session: {
          ...SessionType.Schema,
          args: {
            email: { type: new GraphQLNonNull(GraphQLString) },
            password: { type: new GraphQLNonNull(GraphQLString) },
          },
          resolve: SessionType.create,
        },
      }
    },
  }),
})

types Proxy

const types = new Proxy(
  {},
  {
    get: (target, name) => {
      if (!target[name])
        throw new Error(
          `Attempting to access a type (${name}) that doesn't exist inside one of your ModelType subclasses`
        )
      return target[name]
    },
  }
)

@southpolesteve
Copy link
Author

@timhuff interesting approach!

Fwiw we've actually started moving away from this at Bustle. We saw some big benefits from splitting resolvers entirely from the schema definition. When I wrote the above doing that wasn't really possible since things like graphql-tools didn't support all the features of full JS defined types/schemas. Going forward our preference is to write schemas as .graphql files, build a resolverMap object and then combine the two with graphQL tools.

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.

4 participants