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

getDirectiveDeclaration return type incompatibility in v5 with Apollo Server #1408

Closed
kachkaev opened this issue Apr 25, 2020 · 8 comments
Closed

Comments

@kachkaev
Copy link

kachkaev commented Apr 25, 2020

👋 I'm opening this issue after upgrading deps in my project to latest and discovering this:

class MyDirective extends SchemaDirectiveVisitor {
  // ...
}

const apolloServer = new ApolloServer({
  resolvers,
  schemaDirectives: {
    myDirective: MyDirective
  }
  typeDefs,
  // ...
})

Type '{ myDirective: typeof MyDirective; documentField: typeof DocumentField; documentFieldWithBoolean: typeof DocumentFieldWithBoolean; documentFieldWithLocale: typeof DocumentFieldWithLocale; ... 7 more ...; richText: typeof RichText; }' is not assignable to type 'Record<string, typeof SchemaDirectiveVisitor>'.
  Property 'myDirective' is incompatible with index signature.
    Type 'typeof MyDirective' is not assignable to type 'typeof SchemaDirectiveVisitor'.
      The types returned by 'getDirectiveDeclaration(...)' are incompatible between these types.
        Type 'GraphQLDirective | null | undefined' is not assignable to type 'GraphQLDirective'.
          Type 'undefined' is not assignable to type 'GraphQLDirective'.ts(2322)
graphql-tools@5.0.1-afe7d4a.0 (or 5.0.0)
apollo-server-micro@2.12.0

Seems like Apollo Server does not expect null | undefined as return type here:

// Override this method to return a custom GraphQLDirective (or modify one
// already present in the schema) to enforce argument types, provide default
// argument values, or specify schema locations where this @directive may
// appear. By default, any declaration found in the schema will be returned.
public static getDirectiveDeclaration(
directiveName: string,
schema: GraphQLSchema,
): GraphQLDirective | null | undefined {
return schema.getDirective(directiveName);
}

From Apollo Server docs:

Either way, if the visitor returns a non-null GraphQLDirective from getDirectiveDeclaration, that declaration will be used to check arguments and permissible locations.


Downgrading to graphql-tools@^4.0.8 helps. Here is the method signature there:

// Override this method to return a custom GraphQLDirective (or modify one
// already present in the schema) to enforce argument types, provide default
// argument values, or specify schema locations where this @directive may
// appear. By default, any declaration found in the schema will be returned.
public static getDirectiveDeclaration(
directiveName: string,
schema: GraphQLSchema,
): GraphQLDirective {
return schema.getDirective(directiveName);
}

How shall we resolve this type incompatibility?

@ardatan
Copy link
Owner

ardatan commented Apr 25, 2020

I think Apollo's dependencies are out of date because that signature looks good to me(the item might not be in the map) So instead of mixing Apollo and graphql-tools while generating schema. You can use only graphql-tools to generate your schema. So you can generate your schema with schemaDirectives using makeExecutableSchema from graphql-tools then pass generated schema to Apollo Server at the end .

@ardatan ardatan closed this as completed Apr 25, 2020
@kachkaev
Copy link
Author

kachkaev commented Apr 25, 2020

I see. Thanks for the hint @ardatan, I’m sure it will help others too!

Here is a working solution (for graphql-tools@5.0.1-afe7d4a.0, not for 5.0.0):

+ import { makeExecutableSchema } from "graphql-tools";

  const apolloServer = new ApolloServer({
-   resolvers,
-   schemaDirectives: {
-     myDirective: MyDirective
-   }
-   typeDefs,
+   schema: makeExecutableSchema({
+     resolvers,
+     schemaDirectives,
+     typeDefs,
+   }),
    // ...
  })

Thanks again! 🙌

@ardatan
Copy link
Owner

ardatan commented Apr 25, 2020

Isn't it working with 5.0.0?

@kachkaev
Copy link
Author

Getting

Type '{ myResolver: typeof MyResolver; documentField: typeof DocumentField; documentFieldWithBoolean: typeof DocumentFieldWithBoolean; documentFieldWithLocale: typeof DocumentFieldWithLocale; ... 7 more ...; richText: typeof RichText; }' is not assignable to type '{ [name: string]: typeof SchemaDirectiveVisitor; }'.
  Property 'myResolver' is incompatible with index signature.
    Type 'typeof MyResolver' is not assignable to type 'typeof SchemaDirectiveVisitor'.
      Construct signature return types 'MyResolver' and 'SchemaDirectiveVisitor<TArgs, TContext>' are incompatible.
        The types of 'args' are incompatible between these types.
          Type '{ [name: string]: any; }' is not assignable to type 'TArgs'.
            '{ [name: string]: any; }' is assignable to the constraint of type 'TArgs', but 'TArgs' could be instantiated with a different subtype of constraint '{}'.

@ardatan
Copy link
Owner

ardatan commented Apr 25, 2020

Does MyResolver extend the class SchemaDirectiveVisitor from graphql-tools or apollo-server?
And also as you can see graphql-tools is out of date in here https://github.com/apollographql/apollo-server/blob/master/packages/apollo-server-core/package.json#L43
So it would be better to open an issue on Apollo's repository to update dependencies to the latest version of graphql-tools.

@kachkaev
Copy link
Author

kachkaev commented Apr 25, 2020

Did a project-wide search for SchemaDirectiveVisitor, only seeing

import { SchemaDirectiveVisitor } from "graphql-tools";

One more small patch I had not make to avoid resolves being incompatible with what graphql-tools / makeExecutableSchema expects:

import { mergeResolvers, ResolversDefinition } from "@graphql-toolkit/schema-merging";

const resolversForX: ResolversDefinition<ResolverContext> = { /* */ }
const resolversY: ResolversDefinition<ResolverContext> = { /* */ }

const resolvers = mergeResolvers<
  ResolverContext,
  ResolversDefinition<ResolverContext>
>([
  resolversForX,
  resolversForY,
]) as IResolvers<any, ResolverContext> // <-------------- 👀

Otherwise, seeing

Type 'ResolversDefinition<ResolverContext>' is not assignable to type 'Record<string, GraphQLScalarType | (() => any) | Record<string, ReactText> | IResolverObject<any, ResolverContext, any> | IResolverOptions<...>> | Record<...>[] | undefined'.
  Type 'ResolversFactory<ResolverContext>' is not assignable to type 'Record<string, GraphQLScalarType | (() => any) | Record<string, ReactText> | IResolverObject<any, ResolverContext, any> | IResolverOptions<...>> | Record<...>[] | undefined'.
    Type 'ResolversFactory<ResolverContext>' is missing the following properties from type 'Record<string, GraphQLScalarType | (() => any) | Record<string, ReactText> | IResolverObject<any, ResolverContext, any> | IResolverOptions<...>>[]': pop, push, concat, join, and 27 more.ts(2322)
Interfaces.d.ts(231, 5): The expected type comes from property 'resolvers' which is declared here on type 'IExecutableSchemaDefinition<ResolverContext>'

@yaacovCR
Copy link
Collaborator

@kachkaev Not sure if you are all ready for this, but you should be able to test using schema generation with makeExecutableSchema and passing to Apollo server via schema property, with preserved backwards compatibility, using the new scoped packages:

@graphql-tools/schema
@graphql-tools/utils
etc

as necessary

via canary tag. Would be extremely helpful to us!

Example project using new scoped packages: https://github.com/yaacovCR/apollo-stitcher/blob/master/package.json

v6 API in progress: https://www.graphql-tools.com/docs/introduction

@yaacovCR
Copy link
Collaborator

If you can reproduce the errors above with a minimal example, I think would be appropriate to open a new issue to see if we can help fix it if at all possible....

Would be super helpful.

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

3 participants