Revert "Remove all 'instanceof GraphQLSchema' checks"

[leebyron]

Reverts #371

Now have a better understanding of why those invariants were there - because instanceof is used in many other places, removing these doesn't allow the use of GraphQLSchema defined by other versions of graphql-js because eventually checks like instanceof GraphQLList or instanceof GraphQLObjectType will fail for the same reasons - but at that point the produced errors may be far more mysterious. These invariants were added to at least give a clear error message early on.

If we want to enable this, we'll have to completely remove instanceof.

[helfer]

I see. It would have enabled modifying things that build a schema while still using the original GraphQL-JS types, but clearly it makes more sense to do this properly. I'll work on it as soon as I have time.

[helfer]

@leebyron I see this is going to be quite a lot of work, because instanceof is used almost ubiquitously.

I'd like to get started on this, but since it's going to be a major change, I'd like to have your thumbs up about the approach first:

I think the most straightforward way to implement all these checks is to replace the instanceof call with a function that checks if an object is of a certain type. Rather than using duck typing, I think it's worth adding a __type field to each of the types, and just checking for equivalence of that. Thoughts?

[leebyron]

Something like that is probably right. To better match the AST maybe we call it "kind" (this also avoids objects which model types having a field called type)

One of the biggest challenges will be getting flow error checking to behave correctly. Currently it relies on instanceof checks to resolve unions

[sheerun]

What about:

1. Creating interfaces for each type like GraphQLSchemaInterface
2. Making actual classes like GraphQLSchema to inherit from interfaces
3. Replacing instanceof checks to interfaces instead of actual classes
4. Requiring external modules to inherit from interfaces

[leebyron]

Can you expand on what you mean by interface?

[helfer]

I think by interface he means the way interfaces are used in languages like Java. Instead of having actual functionality, they define what functions and attributes implementing types need to have.

Since Javascript doesn't have a concept of interfaces, this would essentially come down to making an abstract class for each type, which has no properties, and then extending that for the actual class. That would work for writing custom types, but it still wouldn't allow you to use different versions of GraphQL-JS alongside each other, unless these 'interfaces' were broken out into a separate package and imported from there into GraphQL-JS.

@sheerun Is that an accurate description of your idea?

[leebyron]

Yeah, since JavaScript doesn't have a concept of interfaces or abstract classes I think that's probably a non-starter for replacing instanceof.

[sheerun]

Yes, it's accurate. If flow only supports only instanceof then we need to deal with it.

On the other hand I'm not a fan of nominal typing, I rather prefer something like scala's traits or haskell typeclasses or go-lang interfaces. i.e. I'd be really glad if you used structural typing instead as it allows for behavior @helfer expects: implementing interface without inheriting class.

In flow realm it means using interfaces instead of implicitly defined types for classes. i.e. never write import type SomeClass, because this way you're restricting yourself to nominal type of SomeClass.

I've prepared gist to demo the idea: https://gist.github.com/sheerun/30dae1da9e74af47ce91ca19d031bfac

[leebyron]

I don't have a problem with nominal types, but agree that structural typing is usually better and more flow encourages that.

The issue here is with replacing instanceof. Unfortunately flow is a static-time-only type checker, so we if we have a flow union type, we need some way to tell flow something about the runtime object to help it understand which type we're using.

e.g.:

class One { oneMethod() { return 'one' } }
class Two { twoMethod() { return 'two' } }
type OneOrTwo = One | Two;

function acceptsUnion(oneOrTwo: OneOrTwo) {
  if (oneOrTwo instanceof One) {
    return oneOrTwo.oneMethod();
  } else {
    return oneOrTwo.twoMethod();
  }
}

This sort of pattern shows up all the time when differentiating between GraphQLObjectType, GraphQLInterfaceType, etc.

So using flow interfaces and regular objects doesn't allow us to use this pattern anymore because we can't use the interface at runtime to disambiguate the union type.

One possible solution is to mirror how the AST works by using a kind property to implement a sort of "tagged union".

type ObjectType = { kind: 'ObjectType', fieldX: Type }
type InterfaceType =  = { kind: 'InterfaceType', fieldY: Type }
type ObjectOrInterfaceType = ObjectType | InterfaceType

function acceptsUnion(type: ObjectOrInterfaceType) {
  if (type.kind === 'ObjectType') { return type.fieldX; }
}

We'll just have to be careful to ensure flow treats this correctly and doesn't introduce any false-positives or false-negatives.

[leebyron]

I have no idea if this will work or not, but worth a try:

class GraphQLObjectType {
  kind: ('ObjectType': 'ObjectType')
}

so we can continue to use the existing type names throughout, but tell flow that there is a property which is of the constant value type 'ObjectType'.

Maybe?

[leebyron]

@samwgoldman ideas?

[sheerun]

Flow supports tagged unions but it seems they only work for type unions, and not for interface unions.

/* @flow */

type ObjectType = {
  kind: 'ObjectType',
  fieldX: string
};

type InterfaceType = {
  kind: 'InterfaceType',
  fieldY: string
};

type ObjectOrInterfaceType = ObjectType | InterfaceType;

function acceptsUnion(type: ObjectOrInterfaceType) {
  if (type.kind === 'ObjectType') {
    return 'Object: ' + type.fieldX;
  } else {
    return 'Interface: ' + type.fieldY;
  }
}

class ObjectImplementation {
  kind: 'ObjectType';
  fieldX: string;

  constructor() {
    this.kind = 'ObjectType';
    this.fieldX = 'foo';
  }
}

class Object2Implementation {
  kind: 'ObjectType';
  fieldX: string;

  constructor() {
    this.kind = 'ObjectType';
    this.fieldX = 'foo';
  }
}

class InterfaceImplementation {
  kind: 'InterfaceType';
  fieldY: string;

  constructor() {
    this.kind = 'InterfaceType';
    this.fieldY = 'bar';
  }
}

console.log(acceptsUnion(new ObjectImplementation()));
console.log(acceptsUnion(new Object2Implementation()));
console.log(acceptsUnion(new InterfaceImplementation()));

[samwgoldman]

I am missing a lot of context here, but just looking at GraphQLSchema in isolation it looks like you could define an interface—let's call itIGraphQLSchema—of which GraphQLSchema instances would be an inhabitant. Some of the return types in methods on GraphQLSchema are also nominal types, so you'd want to make those interfaces as well.

I'm not sure how disjoint unions are related though.