mickhansen / graphql-sequelize

GraphQL & Relay for MySQL & Postgres via Sequelize
MIT License
1.9k stars 172 forks source link

[RFC] inferTypes #609

Closed ssomnoremac closed 4 years ago

ssomnoremac commented 6 years ago

Really love this library; thanks for all the work! I'm coming from Elixir where the Absinthe authors are pushing to consolidate ecto(ORM) models and graphQL type definitions in the same place. When building my schema with graphql-sequelize, I created a helper gqlObjectType that could describe types very succinctly.

const Doctors = gqlHelpers.gqlObjectType('Doctors')(({listOf, single, connection}) =>
    [
        listOf(Rooms),
        single(Users),
        connection(Appointments),
    ]
)

But then I realized that there is no reason why the types and their relationships couldn't be inferred from the models and associations with options set either in the model files or set globally in the inference function.

const types = gqlHelpers.inferTypes(db, { exclude: ['Accounts', 'TaxInfo']})

Something like that. So my questions are:

  1. Is out of scope for this library?
  2. Is this a bad idea?
  3. Has this been discussed in another issue already?
  4. Is the gqlObjectType from the example above a more reasonable DRY tool to add here?
  5. How would the options work as additional metadata in the model class(like indicating that a hasMany should be a connection)?
mickhansen commented 6 years ago

We have something like attributeFields which does what you describe on a model level basis. While i don't think this is necessarily out of scope, expanding to cover associations might move into territory where everyone wants a different flavour, not everybody uses relay connections, etc.

ssomnoremac commented 6 years ago

Right, so my gqlObjectType helper uses attributeFields to cover the fields under the hood, and exposes the listOf and single functions to define relationships that exist already as associations. I haven't even built the connection yet and anyone could choose to use listOf for an array if they don't use relay connections .

mickhansen commented 6 years ago

@ssomnoremac if you could come up with an API proposal to start, that'd be great.

ssomnoremac commented 6 years ago
/**
 * Infers graphQL types with their fields and relations from 
 * Sequelize models and their associations
 * 
 * @name inferTypes
 * @function
 * @param database {Object} of sequelize models
 * @param options {Object} of options
 * @return {Object} of graphQLObjectTypes
 */

inferTypes(database, options = {})

Usage

import db from './models'
import { inferTypes } from 'graphql-sequelize';

const Types = inferTypes(db)

Converts sequelize models to graphQL types including the associations as relations. Say we have a sequelize model for Appointments. All fields and assocations of Appointments will be generated automatically on the inferred graphQLType.

For associations either a relationship to a single instance of the Type is created or an Array of that type depending on the plurality of the association.

Appointments.hasOne(models.Doctors) => Doctor Type
Appointments.hasMany(models.Notes) => new GraphQLArray(Rooms)

so a fragment such as

{
  Appointment: {
    Doctor {
      lastName
    }
    Notes {
      text
    }
  }
}

would yield

{
  Appointment: {
    Doctor: {
      lastName: "Smith"
    }
    Notes: [
      {text: "late"},
      {text: "sick"}
    ]
  }
}

Params

database

models/index.js export i.e., db

options

Array of strings corresponding to the models to exclude from automatic inference

exclude: ['TaxInfo', 'Reservations']

A Function that returns an object whose keys represent individual fields or relations to override/define

example field override/definition

extend: extendHelpers => ({
  Appointments: {
    visitDescription: {
        type: GraphQLString,
        resolve: () -> 'doctors appointment'
    }
  } 
})

param - extendHelpers {single: function, listOf: function, connection: function}

single(Model, options) - creates a single graphQLObject relation of the type created by Model

listOf(Model, options) - creates a graphQLArray relation with the type created by Model

connection(Model, options) - creates a graphQLRelayConnection with the type created by Model

If a model is not listed in exclude, all fields and relations will be merged with other inferred fields and relations on the resulting type. If you exclude the Model then it is up to you to expose any fields or associations in extend.

example relation override

extend: ({single,  connection}) => ({
  Appointments: {
    Client: single(Clients),
    Tools: connection(Tools)
  } 
})

^ Clients and Tools need to be defined as associations on Appointments and not be excluded

options - resolve override perhaps?

janmeier commented 6 years ago
ssomnoremac commented 6 years ago

@janmeier

Perhaps some hooks

Agreed, before and after?

associations should use connections

I've used Relay Modern quite a bit and there's nothing about it that makes using connections for every relation easy. Having pagination is only useful in very select cases, such as list views or dashboards. But I found even then, only one or two out of dozens of relations in my pervious app called for any of the connection features. Arrays were much easier to work with, even in Relay Modern.

So in my option I think we could have an option use connections by default, but I'm even against that because connections are when you generally want to customize the most -- and for that we should be focusing on connection options as defined above. We might, for instance, have a way to include extra connection arguments.

mickhansen commented 6 years ago

@ssomnoremac

Pagination is not the only reason that connections are powerfull, in my opinion. They are generally a nice way to express relation ships.

Say we have two entities, Player and Match, where a player participates in matches. Without connections you might have something like this.

{
  player {
    matches { id }
  }
}

And that works fine for a while, untill you need to know the total matches a player has, and requesting them all and doing a .length isn't viable, so you end up doing:

{
  player {
    totalMatches
    matches { id }
  }
}

But then you end up with a problem when you need to share arguments:

{
  player {
    totalMatches(won: true)
    matches(won: true) { id }
  }
}

And a second thing is, say you want to express how a player performed in the match? Without connections you'd be left with something like:

{
  player {
    matches {
      players { stats, player }
    }
  }
}

Which forces you to reconcile on the frontend. In my opinion connections solve the issue of designing relations quite well:

{
  player {
    matches(won: true) {
      total
      edges {
         stats { ... }
         node { id }
      }
    }
  }
}
ssomnoremac commented 6 years ago

I agree total is perhaps as useful an addition to connections as pagination, but I don't believe the cost of having to nest ALL plural relations in edges and node (in both fragments and in code), not to mention the complexity connections introduce to client store updater logic on the frontend -- that all of that for perhaps dozens of connections outweighs the burden of having to count arrays and filter by won: true on the front end.

I'm happy to have the default be connections: true as an option on inferTypes if you feel strongly about it and you feel like the sequelize libraries support it easily enough. How would we then customize the connection arguments? And which would the defaults arguments be? total is not part of the graphQL spec if I'm not mistaken.

However, having done a fair amount of front end and back end work on a fairly large app with Relay Modern I think graphQL servers should not over-optimize for the client, and that was my inspiration for inferTypes in that I believe in the thin query layer for clients is the best approach, I would prefer we default to connections: false, which would allow the developer to pick or add custom connection arguments when explicitly setting which associations map to connections.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.