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

Allow to specify field type as a string #1979

Open
stalniy opened this issue Jun 13, 2019 · 12 comments
Open

Allow to specify field type as a string #1979

stalniy opened this issue Jun 13, 2019 · 12 comments

Comments

@stalniy
Copy link

stalniy commented Jun 13, 2019

Currently

graphql.js expects type option to be an instance of GraphQLType (GraphQLInputObjectType, GraphQLObjectType, etc).
Example:

const AnotherType = new GraphQLObjectType({ ... })
const Type = new GraphQLObjectType({
  name: 'MyType',
  fields: {
    anotherType: { type: AnotherType }
  }
})

Desired

In order to split application into modules (which may have circular references in GraphQL types) it'd be helpful to be able to specify type parameter in fields as a string. Updated example:

const Type = new GraphQLObjectType({
  name: 'MyType',
  fields: {
    anotherType: { type: 'AnotherType' } // AnotherType is referenced by its name
  }
})

Workaround

Currently when nodejs modules have circular reference, I can use ugly workaround: calling require in fields thunk:

const Type = new GraphQLObjectType({
  name: 'MyType',
  fields: () => ({
    anotherType: { type: require('anotherModule').AnotherType } 
  })
})

Implementation details

We can add a restriction, that types which a referenced by their name must be included in types option of GraphQLSchema constructor:

const schema = new GraphQLSchema({
  types: [/* specify types that were referenced by their name */],
  query: ...,
  mutation: ...
  subscription: ...
})

So, then the flow is this:

  1. Walk over types array, add them into typeMap
  2. Walk over Query, Mutation, Subscription recursively and resolve types
@langpavel
Copy link
Contributor

langpavel commented Jun 21, 2019

You can just use thunk. You do not need require inside of this thunk.
What you suggest inside implementation details may still not work properly :-)
(Yes, it may work too, but ...)
Just use thunk and do not use destructuring with require. This will work:

const anotherModule = require('anotherModule');

const Type = new GraphQLObjectType({
  name: 'MyType',
  fields: () => ({
    anotherType: { type: anotherModule.AnotherType } 
  })
})

With bundlers even import should work:

import { AnotherType } from 'anotherModule';

const Type = new GraphQLObjectType({
  name: 'MyType',
  fields: () => ({
    anotherType: { type: AnotherType } 
  })
})

But you must use thunk

@stalniy
Copy link
Author

stalniy commented Jun 21, 2019

There are several reasons why it doesn't work in my case:

  1. I don't like to precompile nodejs code as ES6/7 support in nodejs is pretty good.
  2. I like to use destructuring in nodejs. So, it looks very similar to ES6 imports:
const { AnotherType } = require('anotherModule')

If I follow suggested way, the code will look not consistent across the project. Also the suggested approach may not work in .mjs files (haven't checked).
3. In my application, I have 2 kinds of GraphQL types:

  • static - defined by developer
  • dynamic - defined by user and created at runtime
    There are usecases which require to link 2 kinds of types together. Dynamic types are first built at server startup time, so it's hard to get references to it.

Currently I use nexus to build my static schema and pure graphql-js to build dynamic schema. nexus allows to refer to types by string and this is how I currently link dynamic and static types together.

The issue with nexus is that in the recent version, they started to walk recursively over the whole graph of types (the same way as graphql-js does) and actually double the time to create GraphQLSchema instance (nexus schema is generated in 12 times slower than graphqljs schema). That's why I'm looking for alternatives :)

@langpavel
Copy link
Contributor

langpavel commented Jun 21, 2019

  1. I don't like to precompile nodejs code as ES6/7 support in nodejs is pretty good.

That is ok. Nobody enforces you.

  1. I like to use destructuring in nodejs. So, it looks very similar to ES6 imports:

BTW did you add thunk for field everywhere? It may work even with require destructuring.

(NOTE: By destructuring you destroy circular reference workaround in node.
Destructuring in this case is not syntactic sugar. It will rebound actual value of properties to new name.)

If I follow suggested way, the code will look not consistent across the project. Also the suggested approach may not work in .mjs files (haven't checked).

Why not? It will work — because import creates live binding. It's more like
import {exportName} from 'modul'; is more like const modul = require('modul'); modul.exportName (It's not that simple, but principle is same)
(One nice article: What do ES6 modules export?)

Ok, why I'm against this: I cannot imagine how to implement this nicely. Try open PR and play with it ;-)

@stalniy
Copy link
Author

stalniy commented Jun 22, 2019

I see now that it's not easy to work with strings at least right now it will require a lot of changes.

What if we introduce a new type GraphQLRefType(refName) which is resolved to real type during schema construction?

Updated

const { GraphQLObjectType, GraphQLRefType } = require('graphql')

const Type = new GraphQLObjectType({
  name: 'MyType',
  fields: {
    anotherType: { type: new GraphQLRefType('AnotherType') } // AnotherType is referenced by its name
  }
})

const AnotherType = new GraphQLObjectType({
  name: 'AnotherType',
  fields: {
    ....
  }
})

const schema = new GraphQLSchema({
  types: [Type, AnotherType]
})

@langpavel
Copy link
Contributor

langpavel commented Jun 22, 2019

And now compare with this:

const { GraphQLObjectType } = require('graphql')

const Type = new GraphQLObjectType({
  name: 'MyType',
  fields: () => ({
    anotherType: { type: AnotherType }
  })
})

const AnotherType = new GraphQLObjectType({
  name: 'AnotherType',
  fields: {
    ....
  }
})

It is perfectly fine implementation

@stalniy
Copy link
Author

stalniy commented Jun 23, 2019

It’s fine while types in the same file :)

I prefer code first approach to build schema because it’s easier to support and has less boilerplate.

Also I structure my app by independent modules.

In such cases, it’s sometimes useful to have some sort of indirection. It’s kind of simple DI container for types based on GraphQSchema. Also it would help to implement later something like GraphQLExtendType which could allow different modules to extend types of another module.

After trying to overcome some implementation challenges now I see a conceptual issue:
nexus allows to configure type factories which at the schema creation time are converted into GraphQL types. So, different graphql schema instanced can be built on top of the same configuration but with different types in memory.

graphql code first approach is implemented differently. Basically it says - create types, link them together, later when you create 2 different GraphQlSchema instances they share the same GraphQL types in memory, so it’s hard to provide context dependent information for different types from GraphQlSchema (i.e., aggregation point). Thus it’s hard to implement such things like reference to another type by string or GraphQLExtendType (similar to nexus's extendType).

So, if you don’t plan to do something with this, then ticket may be closed.

However the minimum change which I would like to see is the possibility to extend GraphQLSchema class and override type iteration logic, so libraries like nexus can provide prebuilt typeMap and GraphQLSchema will not do the work which was done by a wrapper class (I mean deep types traversal).

@stalniy
Copy link
Author

stalniy commented Jun 23, 2019

Added this snippet, so it's easier to understand what I want when saying "can provide prebuilt typeMap".
Currently these lines of GraphQLSchema cannot be overriden without relying on implementation details in that lines. So, what I suggest is to move that lines into GraphQLSchema methods, so child class can override that logic

const { GraphQLSchema } = require('graphql')

class MyGraphQLSchema extends GraphQLSchema {
   _traverseTypes(types) {
      // returns typeMap
      // line here https://github.com/graphql/graphql-js/blob/master/src/type/schema.js#L184
   }

   _traverseDirectives(directives) {
      // returns typeMap for directives
     // https://github.com/graphql/graphql-js/blob/master/src/type/schema.js#L187
   }

   _buildTypesImplementationIndex() {
       // returns this._implementation
       // lines https://github.com/graphql/graphql-js/blob/master/src/type/schema.js#L196-L211
   }
}

@sibelius
Copy link

I'm trying to split my types in many packages and it is hard to do using type instances when you can't avoid circular references in types (most GraphQL schemas that I know).

thunk solves part of this, but it makes it hard to add or remove fields to existing types

you can do something like this

addFields(graphqlObjecttypes, newFields) because it would cause a circular dep to resolve the thunk when adding new fields

we could have a helper to handle both string and graphql object type

export const getObjectType = (typeOrString) => {
   if (typeOrString instanceof GraphQLObjectType) {
     return typeOrString;
   }

    return typeMap[typeOrString];
}

@stalniy
Copy link
Author

stalniy commented Dec 15, 2020

So, is this something you guys want to see in graphql.js? just don't want to waste time in case you are against it :)

@sibelius
Copy link

can you send a PR for this?

@stalniy
Copy link
Author

stalniy commented Dec 19, 2020

Yes, I can find some free time to do it but it won’t be quick. But, I want to understand that it’s something guys want to see in the codebase and eventually will not reject it because there is already thunk support for this (or something similar).

My free time is currently quite expensive for me because I have a small baby. So I don’t want to throw this time in a trash bin at the end :)

@yaacovCR
Copy link
Contributor

Right now resolving types at runtime is done with strings rather than exact objects, because of bug with schema regenerators that don’t wrap resolveType #2779 (comment)

so it’s odd that you can’t build a schema with strings!

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

No branches or pull requests

5 participants