Abstract type resolution for node definitions

[taion]

The current implementation of nodeDefinitions seems to be a pretty thin wrapper around the object resolution and abstract type resolution. This makes for an interface to nodeDefinitions that feels fairly awkward.

For example, here: https://github.com/relayjs/relay-starter-kit/blob/9f30837849362b65181e6383cfd52ad402a9c00e/data/schema.js#L52 - it feels like it'd be much easier to do something like return {type: userType, value: getUser(id)} instead of separately specifying the type resolver in a different function below. I realize I can get something like a more decoupled syntax if I use isTypeOf, but it seems strange to have to have any sort of explicit type resolution logic if I know a priori the concrete type at the time that I resolve the node value.

Is there any chance of modifying the implementation or the spec in such a way that it would be possible to simultaneously resolve both the result and the concrete type for abstract types, or at least add some wrappers for doing so for Relay nodes?

[KyleAMathews]

Also I don't use typed objects for passing around data—afaik this is relatively uncommon in the JS world. So other than inspecting field names, I don't have a way to know what type an object is.

[taion]

Same, actually - I'm using JSON API so I just branch obj.type. Same thing though.

[skevy]

I'm passing around immutable.js iterables everywhere in my app, so I have a prepareForGraphQL function that I call wherever I resolve an object. It looks like:

export function prepareForGraphQL(arrayOrObjOrImmutable, typeName, customTransformer) {
  const transformer = (obj) => {
    if (customTransformer) {
      obj = {
        ...obj,
        ...customTransformer(obj)
      };
    }

    return {
      $$typeof: Symbol.for(typeName),
      ...obj
    };
  };

  let arrayOrObj;

  if (Iterable.isIterable(arrayOrObjOrImmutable)) {
    arrayOrObj = arrayOrObjOrImmutable.toJS();
  } else {
    arrayOrObj = arrayOrObjOrImmutable;
  }

  if (Array.isArray(arrayOrObj)) {
    if (arrayOrObj.length === 0) return null;
    return arrayOrObj.map(transformer);
  }

  if (!arrayOrObj) return null;

  return transformer(arrayOrObj);
}

And an isTypeOf function that looks like:

export function isTypeOf(typeName) {
  return (obj) => obj.$$typeof === Symbol.for(typeName);
}

I don't use the second argument of nodeDefinitions, and instead rely on isTypeOf in my GraphQLObjectType definitions (which of course calls my custom isTypeOf function). I actually "register" my types with a registry, so that I don't have to have to deal with any weird circular dep issues. That piece looks like:

const TYPE_REGISTRY = {};

export function registerType(type, resolveById) {
  TYPE_REGISTRY[type.name] = {
    type,
    resolveById
  };
  return type;
}

export const {nodeInterface, nodeField} = nodeDefinitions(
  (globalId, info) => {
    const { type, id } = fromGlobalId(globalId);
    let obj = TYPE_REGISTRY[type] ? TYPE_REGISTRY[type].resolveById(id, info) : null;
    return obj;
  }
);

// and then, in my individual type files

const userType = registerType(
  new GraphQLObjectType({
    name: 'User',
    fields: () => ({...}),
    isTypeOf: isTypeOf('USER'),
    interfaces: () => [nodeInterface]
  }
, userFetcher);

This is not the end-all-be-all way to solve this issue, but I'd thought I'd share for both discussion and to provide insight into one possible way to solve the problem.

[taion]

Isn't that less performant because it requires iterating through every candidate type? https://github.com/graphql/graphql-js/blob/f9a287754898faf850101045f7d63000ea1b2159/src/type/definition.js#L584

[skevy]

Yes. But 1) it's only looping through types that implement the nodeInterface and 2) even if you had like 50 node types (which seems like a lot, even for a large app), would you really ever notice the difference?

But in general, yes. You're right. So it may be worth implementing my own resolver and not rely on isTypeOf.

[taion]

Huh? If you have a type registry, don't you trivially have a type resolver that just does a map lookup into the type registry?

This both lets you avoid circular imports if you split up your schema, and gives you an efficient ubiquitous type resolver.

I'd argue this is actually a better pattern than having e.g. a resolveWithType method for abstract types.

[nickretallack]

I'm surprised Relay doesn't do this mapping for you. I mean, you already have to name your GraphQLObjectType. It'd be nice if globalIdField could introspect on that. And then if you could give each GraphQLObjectType that implements nodeInterface some sort of resolver method that takes an ID, nodeDefinitions could introspect on that for its first argument. Not entirely sure how the second argument to nodeDefinitions is used though. Perhaps it could have the same pattern, where you associate a GraphQLObjectType with a JavaScript object type and it introspects that too, but like you said, who knows how you'd want this mapping to work in practice?

I guess you could do all this manually, by building your own registry, but the schema objects are already a registry, so it'd be nice if you could toss in this extra information.

I suppose the reason this isn't automatic is because people may want to use their own ID schemes in place of globalIdField/fromGlobalId.

[taion]

After some use, I'm getting enough out of a central node registry anyway that I don't think this is really a relevant ask. Thanks!

[ryanblakeley]

@taion I'm trying to split up my Types (and Mutations) into their own files. Do you have an example of a schema that has been split up?

[taion]

Just put each of them in their own file, and use thunks for fields.

[ryanblakeley]

@taion thanks for the quick response.

I'm not familiar with thunks. But I was not under the impression that control flow (or async) was the cause of my troubles (of course, I could be wrong). The error I'm getting, seen here graphql/graphql-js#146, seems to be related to calling the new GraphQLObjectType constructor in the dedicated Type file.

I am stabbing in the dark at how to avoid that. Are thunks the answer to that problem? Since your comment in this issue is just about the only mention of splitting up a GraphQL schema I can find anywhere, I thought maybe you knew of an example.

[taion]

You want to do it like this:

// ./src/schema/types/parent.js
import { GraphQLObjectType } from 'graphql';

import ChildType from './ChildType';

export default new GraphQLObjectType({
  name: 'Parent',
  fields: () => ({
    parentId: {
      type: GraphQLString,
      resolve: ({id}) => id,
    },
    child: {
      type: ChildType,
    },
  }),
};

// ./src/schema/types/child.js
import { GraphQLObjectType } from 'graphql';

import ParentType from './ParentType';

export default new GraphQLObjectType({
  name: 'Child',
  fields: () => ({
    parentId: {
      type: GraphQLString,
      resolve: ({id}) => id,
    },
    parent: {
      type: ParentType,
    },
  }),
};

[ryanblakeley]

Thanks, that makes sense.

If you end up importing schema/types/child.js in more than one parent, isn't that going to be calling the new GraphQLObject constructor repeatedly, triggering an error?

Also, where would nodeInterface be constructed and how would it be assigned as the interface for the ObjectTypes living in separate files?

[taion]

Take a look at https://gist.github.com/taion/d161a58b9f7381d8fa9c.

This lets me just make my Node definitions be e.g.:

// ./src/schema/node.js
import { nodeDefinitions } from 'graphql-relay';

import { idFetcher, typeResolver } from './registry';

export const { nodeInterface, nodeField } = nodeDefinitions(
  idFetcher, typeResolver
);

The item fetching stuff is fairly specific to how my REST backend is laid out, in that I can directly map my GraphQL type names to the REST endpoints most of the time, but the basic idea should be fairly general. To avoid code duplication, I'll mostly use the exported getEndpoint from registry.js to do data fetching in general.

At some point I'll properly open source this as a library once I figure out how to make fully generic, but don't hold your breath there.

[taion]

The API assumption I have baked in there is that if I have a type named Widget, the corresponding endpoints on my REST backend will be /api/widgets and /api/widgets/:widgetId.

[ryanblakeley]

Thanks. I'm definitely out of my depth.

It looks like this addresses issues with nodeDefinitions. But I'm not sure how this avoids the repeated constructor calls issue when a Type is a field on more than one other Type. Does it also address that?

It seems like this graphql-relay-js repo, on its own, doesn't support splitting up a schema. Is that truly missing?

[taion]

There's no repeated constructor call with e.g. the pattern I have above. The object types are only constructed once; it's just the calculation of the field types that's deferred.

In principle you can split up a schema however you want using the patterns currently available in graphql-relay. In practice I think centralizing type resolution and node data fetching like I have in my registry is a much more sane approach for a schema of any meaningful size, but it's hard to set up an abstraction there that's both generic and useful - hence why it's left up to us in userspace.

[ryanblakeley]

Does this also cover splitting up connection types, edge types, and mutations? I imagine keeping all the connection definitions in one file and splitting up mutations.

[taion]

I've been going whole hog and putting each of my connections in a separate file. Why not, right?

[ryanblakeley]

Sure, why not. Do you register those connection and edge types and mutations the same way you register object types?

And is this how you end up exporting an object type?

export default registerType(MyType);

Or how does your registry pattern replace this pattern from the examples:

var {nodeInterface, nodeField} = nodeDefinitions(
  (globalId) => {
    var {type, id} = fromGlobalId(globalId);
    if (type === 'Viewer') {
      return getViewer();
    } else if (type === 'User') {
      return getUser(id);
    } else {
      return null;
    }
  },
  (obj) => {
    if (obj instanceof Viewer) {
      return ViewerType;
    } else if (obj instanceof User) {
      return UserType;
    } else {
      return null;
    }
  }
);

[taion]

That's how I export, yes.

I don't register those types because they aren't nodes.

[ryanblakeley]

Ok, so its safe to just import them as normal modules I guess. graphql-relay doesn't barf when you call connectionDefinitions on the same connection more than once(?)

[taion]

Let's take this off the graphql-relay-js issue tracker - it's not a great medium for this. Try catching someone on the Relay channel on Reactiflux.

[ryanblakeley]

Sure thing, thanks a bunch for answering all those questions.

[kevinSuttle]

I ended up here trying to track down a solution to an issue with circular dependencies and Interface type resolutions. Purely an issue with graphql-js, I know, as I'm not using Relay in this app, but it seems like this is a very common problem.

If you are forced to make a registry for every Schema, what's the point of Interfaces? That's essentially a registry, conceptually.

Related issues:
graphql/graphql-js#277
#66

See also: @wincent's implementation: https://github.com/wincent/masochist/blob/master/app/src/server/schema/definitions/node.js

[taion]

I've found interfaces useful because they let me write Relay components that only expect an object of the interface type, and only deal with fields there.

[kevinSuttle]

@taion Should we tack this discussion onto one of the issues I referenced?