Skip to content

Make source type of isTypeOf unknown #3385

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

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

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