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

Interface confusion #338

Closed
kevinSuttle opened this issue Mar 30, 2016 · 16 comments
Closed

Interface confusion #338

kevinSuttle opened this issue Mar 30, 2016 · 16 comments

Comments

@kevinSuttle
Copy link

carried over from graphql/graphql-relay-js#25 (comment)

x.resolveType field type must be Output Type but got: undefined.
Error: X may only implement Interface types, it cannot implement: undefined.

I've run into one of these or a circular dependency a number of times following the docs and examples in the wild.

It seems developers always end up needing to implement a type registry, which I thought a Schema was.

https://facebook.github.io/graphql/#sec-Interface-type-validation The docs here are almost empty, but even the one caveat makes very little sense. The section above it doesn't even mention isTypeOf, valueOf, or resolveType. I've found these on GitHub or StackOverflow.

Using the _schema introspection, Interfaces themselves don't appear as types (at least in my experience), which seems confusing, as does the following result:

   {
          "name": "__Type",
          "interfaces": []
        },
        {
          "name": "Boolean",
          "interfaces": null
        },

Another example: An app I'm working on will behave very differently depending on if I wrap a single Interface type with Array brackets as well.

interfaces: graphicType vs interfaces: [graphicType]
If do the latter, I get Error: Box may only implement Interface types, it cannot implement: undefined.

If I do the former, all the types that implement that interface return null, and get this in GraphiQL:

"errors": [
    {
      "message": "Expecting a function in instanceof check, but got [object Object]"
    }
  ]

Another confusing part of the spec is that interfaces are just fields. e.g. neither of the following work.
const boxType = new GraphicType({
const boxType = graphicType({

Babel throws an error.

var boxType = (0, _graphic2.default)({
                                     ^

TypeError: (0 , _graphic2.default) is not a function

Instead, you need to make it a predefined type.

const boxType = new GraphQLObjectType({
...
// which you then have to return as the implemented type
isTypeOf: (value) => value instanceof boxType,

Can you see how confusing that is, especially coming from an OOP perspective?

One suggestion from reactiflux discord was to put the Interface and its implementing types into one file, but that smells funny to me.

@dschafer
Copy link
Contributor

Hm, sounds tricky. It's tough to advise here without seeing more of the code; can you post an example where you get those two error types?

https://facebook.github.io/graphql/#sec-Interface-type-validation The docs here are almost empty, but even the one caveat makes very little sense. The section above it doesn't even mention isTypeOf, valueOf, or resolveType. I've found these on GitHub or StackOverflow.

Yeah, that's the specification for interfaces rather than documentation on how to use them. You're right in that our documentation for interfaces is mostly by example right now.

Using the _schema introspection, Interfaces themselves don't appear as types (at least in my experience), which seems confusing, as does the following result:

Note that #327 was a breaking change here on how interfaces are added to a schema; the test changes in there should help clarify the usage now. Note that interfaces is empty for __Type because __Type is an object type without any interfaces; interfaces is null for Boolean because Boolean is a scalar, and scalars must return null for interfaces as per http://facebook.github.io/graphql/#sec-Scalar

Another confusing part of the spec is that interfaces are just fields. e.g. neither of the following work.

Hm, I'm not sure why you would want to instantiate an interfaces type; can you go into more detail?

put the Interface and its implementing types into one file, but that smells funny to me

Yeah, you shouldn't need to do so; when defining an object type, you can pass a thunk to the interfaces field, which should avoid most circular dependencies. The node interface in https://github.com/graphql/swapi-graphql/blob/master/src/schema/relayNode.js is implemented by https://github.com/graphql/swapi-graphql/blob/master/src/schema/types/person.js which might be a useful example.

Hope this helps!

@kevinSuttle
Copy link
Author

Thanks @dschafer. Let me expound a bit.

Here's the Interface.

import {
  GraphQLEnumType,
  GraphQLNonNull,
  GraphQLString,
  GraphQLInterfaceType } from 'graphql';

import iconType from './icon';
import logoType from './logo';

const graphicFileFormatsEnum = new GraphQLEnumType({
  name: 'graphicFileFormats',
  description: 'The various file formats in which graphics can be delivered.',
  values: {
    SVG: {
      value: 'svg',
      description: 'Scalable Vector Graphics format',
    },
    PNG: {
      value: 'png',
      description: 'Portable Network Graphics format',
    },
  },
});

const graphicType = new GraphQLInterfaceType({
  name: 'graphic',
  description: 'A visual graphic used in product UIs.',
  fields: () => ({
    name: {
      type: new GraphQLNonNull(GraphQLString),
      description: 'The name of the graphic',
    },
    format: {
      type: graphicFileFormatsEnum,
      description: 'The various file formats a graphic can be delivered in.',
    },
    resolveType: (value) => {
      if (value instanceof logoType) {
        return logoType;
      }
      else if (value instanceof iconType) {
        return iconType;
      }
      else {
        return null;
      }
    },
  }),
});

export { graphicFileFormatsEnum };
export default graphicType;

And 2 implementing types:

import {
  GraphQLNonNull,
  GraphQLString,
  GraphQLObjectType } from 'graphql';

import graphicType, { graphicFileFormatsEnum } from './graphic';

const logoType = new GraphQLObjectType({
  name: 'Logo',
  description: 'An image representing an offering.',
  interfaces: graphicType,
  fields: () => ({
    name: {
      type: new GraphQLNonNull(GraphQLString),
      description: 'The name of the logo.',
    },
    format: {
      type: graphicFileFormatsEnum,
      description: 'The various file formats a graphic can be delivered in.',
    },
  }),
  isTypeOf: (value) => value instanceof logoType,
});

export default logoType;
import {
  GraphQLEnumType,
  GraphQLNonNull,
  GraphQLString,
  GraphQLObjectType } from 'graphql';

import graphicType, { graphicFileFormatsEnum } from './graphic';

const iconCollectionNamesEnum = new GraphQLEnumType({
  name: 'iconCollections',
  description: 'The various types of collections that icons belong to.',
  values: {
    ACTION_BASED: {
      value: 'action-based',
      description: 'Icons used in action-based UI components',
    },
    OBJECT_BASED: {
      value: 'object-based',
      description: 'Icons representing common objects in UIs.',
    },
    FORMATTING: {
      value: 'formatting',
      description: 'Icons used for content creation and interactivity',
    },
  },
});

const iconType = new GraphQLObjectType({
  name: 'Icon',
  description: 'A visual graphic asset for UI components.',
  interfaces: graphicType,
  fields: () => ({
    name: {
      type: new GraphQLNonNull(GraphQLString),
      description: 'The name of the icon found in the IBM Design Language.',
    },
    collection: {
      type: iconCollectionNamesEnum,
      description: 'The name of the collection the icon belongs to.',
    },
    format: {
      type: graphicFileFormatsEnum,
      description: 'The various file formats a graphic can be delivered in.',
    },
  }),
  isTypeOf: (value) => value instanceof iconType,
});

export { iconCollectionNamesEnum };
export default iconType;

Hope that helps.

@dschafer
Copy link
Contributor

I see. So if you implement isTypeOf on all of the object types, you don't need to implement resolveType, which should fix the circular dependency issues.

Looking at the implementation of isTypeOf:

isTypeOf: (value) => value instanceof iconType,

When I think about GraphQL schemas and types, I generally think about them at two different layers: there's a business object, and there's the type that wraps it. The object that gets passed into the resolve function for a field will generally be the business object (and the resolve function will often return a business object as well). That same business object is what is passed into isTypeOf. So this feels like it's mixing the layers: we're asking if a business object is an instance of the GraphQL type.

So if you have a field that returns a Graphic that does this:

someGraphic: {
  type: new GraphQLNonNull(Graphic),
  resolve: (obj) => getSomeGraphic()
},

Then the question that we're trying to answer in isTypeOf is "did the thing returned by getSomeGraphic() return a logo"? In either case, it didn't return a iconType or a logoType; it returned some business object representation of an icon or logo.

So I'm not sure isTypeOf: (value) => value instanceof iconType, is doing what you want; instead, I suspect you want to do isTypeOf: (value) => value instanceof Icon, if you have an "Icon" business object class, or the equivalent based on your underlying business logic.

@kevinSuttle
Copy link
Author

Thanks, so, removing the resolveType() function from the Interface, still results in the child types reporting as null. I did attempt to create BOs like you mentioned, following the pattern here, but changed nothing.

As a result, these are failed tests I'm seeing in expect:

 Icon Type
    2) should be able to resolve all icons
    3) should be able to resolve an icon's collection type
    4) should be queried by collection type

  Logo Type
    5) should be able to resolve all logos
    6) should be able to resolve a logo's format
    7) should be able to resolve a logo's name

@kevinSuttle
Copy link
Author

Is there more info I can give to help wade through this confusion?

@leebyron
Copy link
Contributor

leebyron commented Apr 1, 2016

It seems like there are two issues here:

The first is that interfaces on an Object definition accepts an Array, and you provided the type directly, so:

 const iconType = new GraphQLObjectType({
   name: 'Icon',
   description: 'A visual graphic asset for UI components.',
-  interfaces: graphicType,
+  interfaces: [ graphicType ],

This seems like something we should highlight with an early error.

If that doesn't fix the issue, a second problem may be:

import iconType from './icon';
import logoType from './logo';

in your interface file. That's creating the circular dependency and causing those files to be included before the interface is defined.

Another way to solve this might be declaring your interfaces as a thunk, so they're not expected to be a live reference until the full schema is constructed:

 const iconType = new GraphQLObjectType({
   name: 'Icon',
   description: 'A visual graphic asset for UI components.',
-  interfaces: [ graphicType ],
+  interfaces: () => [ graphicType ],

@kevinSuttle
Copy link
Author

Thanks @leebyron. Unfortunately the thunk format is the only suggestion I haven't tried, and it changes nothing. I think I'm missing something more fundamental that's implied by existing implementations or docs.

Error: Icon may only implement Interface types, it cannot implement: undefined.

@leebyron
Copy link
Contributor

leebyron commented Apr 1, 2016

That error implies that whatever is provided to interfaces is the value undefined. Double check (maybe with console.log) that graphicType is in fact what you think it is

@kevinSuttle
Copy link
Author

So here's the weird thing. Graphic is not undefined. I just logged graphicType === undefined and graphicType === null and the server ran without error, after logging 'false' both times. I checked to see if IconType or LogoType was undefined or null. Nope.

I still had the _schema query in GraphiQL, and this time it looked a lot different.

screen shot 2016-04-01 at 4 25 54 pm

The odd thing is, queries for those 2 child types still resolve to null, and I get that odd JS error again, which I see here in the GraphQL source.

screen shot 2016-04-01 at 4 26 42 pm

When I run Object.keys(iconType) or Object.getOwnPropertyNames(iconType) in the icon.js file, I get:
[ 'name', 'description', 'isTypeOf', '_typeConfig', '_interfaces' ]

@eugenehp
Copy link

@kevinSuttle any luck on this?

@kevinSuttle
Copy link
Author

kevinSuttle commented Apr 20, 2016

Honestly, it's a side project, so I had to put it aside for a bit. I need to re-read this post by @clayallsopp again. I think I may be looking for a Union type. https://medium.com/the-graphqlhub/graphql-tour-interfaces-and-unions-7dd5be35de0d#.uw6ndo5hl

@whitmanc
Copy link

whitmanc commented Jun 16, 2016

Declaring interfaces as a thunk as described by @leebyron earlier in the thread resolved the following issue for me: {TYPE} may only implement Interface types, it cannot implement: undefined.

What I had::
interfaces: [NodeInterface]

What I needed:
interfaces: () => [NodeInterface]

For me, this issue started occurring when I moved my node interface definitions into their own file.

Thanks @kevinSuttle for opening the ticket and thanks to @leebyron for your diligence (and solution!)

@jcampbell05
Copy link

The thunk solves this for me also, the examples should be updated to highlight issues with circular dependencies.

@sonewman
Copy link

sonewman commented Sep 3, 2016

This may be resolved now, but looking at the code examples above the value being set to 'interfaces' in the Object Config is a graphql type and it should be an array of types. E.g. 'Interfaces: [graphicType]'

@WickyNilliams
Copy link

Since every graphql type has a unique string name, could this be used to opt-in to an interface or for resolveType? Would avoid circular references

@IvanGoncharov
Copy link
Member

I don't see any actionable items for graphql-js in this issue + it's stale for the last couple of years so I'm closing it.

Since every graphql type has a unique string name, could this be used to opt-in to an interface or for resolveType? Would avoid circular references

@WickyNilliams Thanks for suggestions. You can return names from resolveType as for providing interface names to GraphQLObjectType it will make constructed object to be dependent on GraphQLSchema (to convert the name into type) and will break a lot of functionality in this lib.

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

No branches or pull requests

9 participants