VulcanJS / Vulcan

🌋 A toolkit to quickly build apps with React, GraphQL & Meteor
http://vulcanjs.org
MIT License
7.98k stars 1.88k forks source link

Graphql interfaces #1674

Open ochicf opened 7 years ago

ochicf commented 7 years ago

Hi!

First of all, I want to thank for this module/framework/whatever, it is awesome!

For a module that I'm writing I've encountered the need to use graphql interfaces to provide more flexibility in the types that can be resolved by some fields, and I see that Vulcan does not provide support for them (at least for what I saw). I've been doing some simple hacks under the hood and I've managed to accomplish what I was intending, but before doing a PR I wanted to debate with you guys what do you think.

From the problems that I've encountered, there are a few main issues that must be taken into consideration:

1. How interfaces are created

This can be done in raw graphql and passing it to addGraphQLSchema. It has one problem though: it is mandatory to define the __resolveType resolver that provides the type of the document implementing the interface.

import { addGraphQLSchema, addGraphQLResolvers } from 'meteor/vulcan:core';

addGraphQLSchema(`
  interface HierarchicalInterface {
    _id: String
    parentId: String
    parent: HierarchicalInterface
  }
`);

addGraphQLResolvers({
  HierarchicalInterface: {
    // in the examples I've found they determine the types based on the `obj`
    // properties, but that requires to know the types beforehand. Here I'm just
    // returning the parentType name (don't know if will work for all cases) 
    __resolveType: (obj, context, info) => info.parentType.name,
  },
});

Maybe a createInterface function could be created to simplify this process?

2. How interfaces are passed to createCollection function

In my implementation I've passed an interfaces property in the options parameter, so it is at the same level that collectionName, typeName, schema, etc.; and that property is just an array containing the interfaces names.

import { createCollection } from 'meteor/vulcan:core';

// must have the fields with same name and types required by the interface
// it will also provide the extra `name` field 
import schema from './schema';

export default createCollection({
    collectionName: 'Categories',
    typeName: 'Category',
    interfaces: ['HierarchicalInterface'],
    schema,
    // ... other options, 
});

Maybe the interfaces should be inside the schema object itself?

3. How data is retrieved by fragments

When writing fragments for interfaces it is required to include inline fragments in order to make them really useful, and for now Vulcan does not support them. The problem seems to be that their syntax starts with ... -the same as for subfragments-, and the current code treat everything that starts with ... as subfragments and just breaks when adding an inline-fragment.

import { registerFragment } from 'meteor/vulcan:core';

registerFragment(`
  fragment CategoriesItem on Category {
    _id
    name
    parentId
    parent {
      _id
      ... on Category {
        name
      }
    }
  }
`);

By tweaking the vulcan:lib.lib.modules.fragments.registerFragment function I've managed to fix it and everything seem to work fine. I'm sure it can be done better because I suck when it comes to regex.


That's it, I'll fork the repo, commit the changes and create a PR so you can see what disasters I've written to make this work xD

Looking forward for your thoughts.

SachaG commented 7 years ago

Thanks for the well thought-out post! By the way, maybe you saw this already but although I've never used interfaces in Vulcan, the Voting package does implement a union:

https://github.com/VulcanJS/Vulcan/blob/master/packages/vulcan-voting/lib/graphql.js#L16-L32

So I'm not sure if we need a new createInterface function at this point? Although I'm open to it if you think it can really make things easier.

For passing the interface to createCollection, can you remind me why that's necessary? Where and how would you access collection.interfaces?

And finally for fragments, yes I agree completely, we should support both inline fragments and subfragments. I also suck at regex so don't worry about that :)

Looking forward to the PR!

ochicf commented 7 years ago

Hi Sacha,

Yes I saw the voting package and considered using unions, but the biggest problem I see with them is that you need to know beforehand what types do you want to create the union from (correct me if I'm wrong). This does not comply with the open-closed principle and fails to provide an extensible solution. Of course it still may be a good solution when using them in more closed systems or parts not intended to be reusable.

For example the case of your voting package: with the votable union you're creating a new type-like definition that is the union of two types and that cannot be changed. If you wanted your package to be extended and reused by others, for example to allow others to define more votable types, then interfaces are the way to go. You would define the Votable interface and then implement it in both Post and Comment types, leaving an open door for others to implement their own Votable solutions.

I've found a great article with more insight in this topic: GraphQL Tour: Interfaces and Unions by Clay Allsopp.

Note that the article it also talks about the interface resolver __resolveType problematic:

The GraphQL execution process will ask the same sorts of questions to interfaces as unions (“hey, what type is this?”), but the difference with interfaces is that an interface may not know all the types that implement it.

[...] GraphQL-JS offers an alternative to interface.resolveType. The GraphQL execution process will first traverse all of the types (defined by schema.types) and try to invoke isTypeOf if it is defined.

It seems like GraphQL-JS adressess this issue by allowing a isTypeOf method for each type calling it for each object in order to determine its actual type.

About why to pass the interfaces in the createCollection, for the moment I'm just accessing them when generating the graphQLSchema in order to set them as type TypeName implements Interface1, InterfaceN.

I'm adding an example package to show how interfaces can be used. For now there is only the simple Categories example of OP, but I can add a more complex where interfaces truly show their potential.

SachaG commented 7 years ago

I think those are all great points. Maybe the voting package could be reimplemented using interfaces then? In any case I agree with everything you've said so far, just let me know if you need anything else from me :)

ochicf commented 7 years ago

Created PR.