Skip to content

Conversation

@ryym
Copy link

@ryym ryym commented Nov 27, 2021

Hi,

Currently the type of source argument in isTypeOf is TSource (the source type of GraphQLObjectType), but I think it is wrong since isTypeOf may be called with a source value of any type that implements the same interface or that belongs to the same union. Therefore I changed the type of source to unknown.

For example, the following code passes type check but results in an error at runtime.

import {
  graphql,
  GraphQLSchema,
  GraphQLObjectType,
  GraphQLString,
  GraphQLInterfaceType,
} from "graphql";

class Human {
  constructor(readonly name: string) {}
}

class Droid {
  constructor(readonly name: string, readonly primaryFunction: string) {}
  serialNumber = () => "-";
}

const CharacterType = new GraphQLInterfaceType({
  name: "Character",
  fields: { name: { type: GraphQLString } },
});

const HumanType = new GraphQLObjectType<Human>({
  name: "Human",
  fields: {
    name: { type: GraphQLString },
  },
  interfaces: [CharacterType],
  isTypeOf: (source) => {
    return source instanceof Human;
  },
});

const DroidType = new GraphQLObjectType<Droid>({
  name: "Droid",
  interfaces: [CharacterType],
  fields: {
    name: { type: GraphQLString },
    primaryFunction: { type: GraphQLString },
  },
  isTypeOf: (source) => {
    // XXX: Here the type of source is inferred as Droid, but it can be Human.
    // In that case this results in error at runtime.
    console.log("debug", source.serialNumber());
    return source instanceof Droid;
  },
});

const QueryType = new GraphQLObjectType({
  name: "Query",
  fields: {
    anyCharacter: {
      type: CharacterType,
      resolve: () => new Human("Han Solo"),
    },
  },
});

const schema = new GraphQLSchema({ query: QueryType, types: [DroidType, HumanType] });
const query = "query Test { anyCharacter { name } }";
graphql({ schema, source: query, rootValue: {} }).then((res) => {
  console.log(res);
  //=> res.errors: [TypeError: source.serialNumber is not a function]
});

Since it can be an any type that implements the same Interface or that belongs to the same Union.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 27, 2021

CLA Signed

The committers are authorized under a signed CLA.

@IvanGoncharov IvanGoncharov added PR: bug fix 🐞 requires increase of "patch" version number PR: breaking change 💥 implementation requires increase of "major" version number and removed PR: bug fix 🐞 requires increase of "patch" version number labels Nov 28, 2021
@IvanGoncharov
Copy link
Member

Changing type to unknown is correct and improves DX however it's a breaking change so I can't merge it in v16.
The good news, we will switch main to "v17 alpha" this Wednesday so I will merge it after the switch.

@IvanGoncharov IvanGoncharov added this to the v17.0.0-alpha milestone Nov 28, 2021
@yaacovCR
Copy link
Contributor

Flagging this for v17.

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

Labels

PR: breaking change 💥 implementation requires increase of "major" version number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants