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

upgrade to graphql-tools@4.0.0 breaks custom schema directives #957

Closed
tsukhu opened this issue Sep 21, 2018 · 18 comments
Closed

upgrade to graphql-tools@4.0.0 breaks custom schema directives #957

tsukhu opened this issue Sep 21, 2018 · 18 comments

Comments

@tsukhu
Copy link

tsukhu commented Sep 21, 2018

On upgrade to graphql-tools@4.0.0 from v3.1.1 causes compile time issues related to SchemaDirectiveVisitor

See reference build log https://travis-ci.org/ERS-HCL/nxplorerjs-microservice-starter/jobs/430812854

The date and auth directives are setup similar to what is explained in the documentation https://www.apollographql.com/docs/apollo-server/features/creating-directives.html

some snippets are given below

  const schema = makeExecutableSchema({
    typeDefs: myTypeDefs,
    resolvers: resolvers,
    schemaDirectives: {
      date: FormattableDateDirective,
      auth: AuthDirective
    }
  });
Property 'date' is incompatible with index signature.
Type 'typeof FormattableDateDirective' is not assignable to type 'typeof SchemaDirectiveVisitor'.

This was working fine till version 3.1.1 but now is breaking.

@hwillson
Copy link
Contributor

From the changelog:

NOTE: graphql 14 includes breaking changes. We're bumping the major version of graphql-tools to accommodate those breaking changes. If you're planning on using graphql 14 with graphql-tools 4.0.0, please make sure you've reviewed the graphql breaking changes list.

This is likely caused by the fact that graphql-js now requires you to define your directives in your schema, before you attempt to use them. For example:

directive @upper on FIELD_DEFINITION

type TestObject {
  hello: String @upper
}

You can likely work around this by pre-defining your directives in your schema, but I'd like to confirm this. If this works, we'll need to update the docs.

@tsukhu
Copy link
Author

tsukhu commented Sep 21, 2018

@hwillson Thank you for your response.

Here is what I am doing

  1. Defining all the schema using graphql scripts https://github.com/ERS-HCL/nxplorerjs-microservice-starter/tree/master/server/graphql/schema
  2. As part of that I have already define the directives https://github.com/ERS-HCL/nxplorerjs-microservice-starter/blob/master/server/graphql/schema/queries.graphql
    In my case
directive @auth(requires: Role = ADMIN) on OBJECT | FIELD_DEFINITION

directive @date(defaultFormat: String = "mmmm d, yyyy") on FIELD_DEFINITION

scalar Date
  1. Then I am setting up the schema

https://github.com/ERS-HCL/nxplorerjs-microservice-starter/blob/master/server/graphql/setupSchema.ts

 const myTypeDefs = importSchema('server/graphql/schema/main.graphql');
  const schema = makeExecutableSchema({
    typeDefs: myTypeDefs,
    resolvers: resolvers,
    schemaDirectives: {
      date: FormattableDateDirective,
      auth: AuthDirective
    }
  });

I guess with the new changes this may not work as these may be required at compile time now rather than runtime . Is my assumption correct.

@fuchen
Copy link

fuchen commented Sep 26, 2018

I have same issue. This error is reported by Typescript compiler, so it has nothing to do with graphql schema.

@hwillson
Copy link
Contributor

@fuchen Any chance you could share a small runnable reproduction that shows this happening?

@fuchen
Copy link

fuchen commented Sep 27, 2018

@hwillson Sorry, I cannot reproduce it anymore. When I saw this error, the cause is { auth: AuthDirectiveVisitor } cannot be assigned to Record<string, typeof SchemaDirectiveVisitor>. But this error is gone now, and I don't know why.

@tsukhu
Copy link
Author

tsukhu commented Oct 1, 2018

@hwillson on investigating further here is what I have found

I am using
"apollo-server": "^2.1.0",
"apollo-server-express": "^2.1.0",

Looks like apollo-server-core depends on v3.1.x of graphql-tools and when we use graphql-tools@4.0.0 there is a signature mismatch. Is there a way to take care of this issue (I did try defining a peer dependency for graphql-tools@4.0.0 but that did not work out.

The IDE also reports this error

Types of parameters 'directiveVisitors' and 'directiveVisitors' are incompatible.
                Type '{ [directiveName: string]: typeof import("e:/workspace/express-microservice-starter/node_modules/apollo-server-core/node_modules/graphql-tools/dist/schemaVisitor").SchemaDirectiveVisitor; }' is not assignable to type '{ [directiveName: string]: typeof import("e:/workspace/express-microservice-starter/node_modules/graphql-tools/dist/schemaVisitor").SchemaDirectiveVisitor; }'.
                  Index signatures are incompatible.

image

@tsukhu
Copy link
Author

tsukhu commented Oct 3, 2018

@hwillson I guess this issue can be closed now
I had added graphql-tools as well as apollo-server . Since apollo-server itself exports graphql-tools , we should not be using any other version to avoid the situation as above. I have removed the direct dependency of graphql-tools from my project and using the SchemaDirectiveVisitor from apollo-server instead

import { SchemaDirectiveVisitor } from 'apollo-server';

@32graham
Copy link

@hwillson I have the issue you mentioned where I'm required to define my directives in my schema despite having it defined in getDirectiveDeclaration. I have a small script that reproduces the issue on this Stack Overflow question. I still have not found a solution other than just defining my directives in my schema.

@fennecbutt
Copy link

fennecbutt commented Feb 8, 2019

Why would they break use of getDirectiveDeclaration!? It was so useful to not require users of custom directives to define them in their own schema...

Could graphql-tools automatically generate the relevant schema declaration for any custom decorators defined using schemaDirectives in makeExecutableSchema for ones that override getDirectiveDeclaration of SchemaDirectiveVisitor?

@ashb
Copy link

ashb commented Apr 12, 2019

@hwillson Nothing in the graphql changelog talks about directives needing to be declared in the schema.

Using https://gist.github.com/benjamn/de4b929ca88283925053a98d03f10ec5#file-schema-directives-using-directives-js as a base.

The problem seems to be that https://github.com/apollographql/graphql-tools/blob/b85137ba81c9bdc5667bced9cad9b1d226ea83fc/src/makeExecutableSchema.ts#L52

validates the directives (and throws an exception), but the directives from passed in schema directives wouldn't be added until line 88 https://github.com/apollographql/graphql-tools/blob/b85137ba81c9bdc5667bced9cad9b1d226ea83fc/src/makeExecutableSchema.ts#L88

Exception stacktrace
at assertValidSDL (node_modules/graphql/validation/validate.js:89:11)  
at Object.buildASTSchema (node_modules/graphql/utilities/buildASTSchema.js:67:34)  
at Object.buildSchemaFromTypeDefinitions (node_modules/graphql-tools/src/generate/buildSchemaFromTypeDefinitions.ts:43:32)
at makeExecutableSchema (node_modules/graphql-tools/src/makeExecutableSchema.ts:52:16)
 at Object.<anonymous>.test (src/lib/directives/auth/index.unit.test.js:17:18)

@hwillson Can you say that by design graphql-tools makeExecutableSchema now(?) needs directives to be declared in the SDL? If so I don't see there point in having getDirectiveDelcaration on the SchemaDirectiveVisitor classes anyomore. Have I missed something

Or is this a bug and the example should still work?

@bionicles
Copy link

hitting this issue ... a bit frustrating, wish i could just use functional programming simple directives, would it be possible to simplify custom directives?

api_1     | Unknown argument "rules" on directive "@auth".
api_1     |     at assertValidSDL (/app/node_modules/graphql/validation/validate.js:108:11)
api_1     |     at Object.buildASTSchema (/app/node_modules/graphql/utilities/buildASTSchema.js:71:34)
api_1     |     at Object.buildSchemaFromTypeDefinitions (/app/node_modules/graphql-tools/src/generate/buildSchemaFromTypeDefinitions.ts:43:32)
api_1     |     at Object.makeExecutableSchema (/app/node_modules/graphql-tools/src/makeExecutableSchema.ts:52:16)
api_1     |     at ApolloServer.initSchema (/app/node_modules/apollo-server-core/src/ApolloServer.ts:477:27)
api_1     |     at new ApolloServerBase (/app/node_modules/apollo-server-core/src/ApolloServer.ts:359:26)
api_1     |     at new ApolloServer (/app/node_modules/apollo-server-express/src/ApolloServer.ts:88:5)

@bionicles
Copy link

here was the cause, for future reference

 directive @auth on FIELD_DEFINITION

needed to specify

 directive @auth(rules: [String]) on FIELD_DEFINITION

i jsut assumed it would work without needing keys (that would be less verbose)

@yaacovCR
Copy link
Collaborator

Duplicate of #1022, folding into #1306

@yaacovCR yaacovCR mentioned this issue Mar 31, 2020
22 tasks
@ivansard
Copy link

ivansard commented Apr 26, 2020

Hey guys,

I'm using apollo-server-express and I'm running into a similar issue.

ERROR in /home/i.sardelic/Desktop/notes/luckynote_client_server/server/src/index.ts
./src/index.ts
[tsl] ERROR in /home/i.sardelic/Desktop/notes/luckynote_client_server/server/src/index.ts(34,5)
      TS2322: Type 'Record<string, typeof import("/home/i.sardelic/Desktop/notes/luckynote_client_server/server/node_modules/apollo-server-express/node_modules/graphql-tools/dist/schemaVisitor").SchemaDirectiveVisitor>' is not assignable to type 'Record<string, typeof import("/home/i.sardelic/Desktop/notes/luckynote_client_server/server/node_modules/apollo-server-core/node_modules/graphql-tools/dist/schemaVisitor").SchemaDirectiveVisitor>'.
  Index signatures are incompatible.
    Type 'typeof import("/home/i.sardelic/Desktop/notes/luckynote_client_server/server/node_modules/apollo-server-express/node_modules/graphql-tools/dist/schemaVisitor").SchemaDirectiveVisitor' is not assignable to type 'typeof import("/home/i.sardelic/Desktop/notes/luckynote_client_server/server/node_modules/apollo-server-core/node_modules/graphql-tools/dist/schemaVisitor").SchemaDirectiveVisitor'.

It's like it's cross-referencing SchemaDirectiveVisitor from two different packages at the same time (apollo-express-core and graphql-tools). That's my assumption if anyone can clarify a bit more I would also be grateful.

Because I am only using a single package, I guess the idea of importing everything from a single package (e.g. graphql-tools) is out for me. Any suggestions on how to approach this?

Any help is greatly appreciated, cheers guys!

@ardatan
Copy link
Owner

ardatan commented Apr 26, 2020

Apollo needs to update the dependencies, there is nothing we can do in this situation.
See #1408 (comment)

@ivansard
Copy link

@ardatan

Thanks for the reply. I hope this is sorted out soon.

Cheers!

@henry-huynh-3508
Copy link

Is there any quick fix for this issue? Custom directives are no longer triggered properly for me.

@eddeee888
Copy link

You can try forcing module resolution in your package.json. Then, delete lock files, node_modules and reinstall modules:

// package.json
{
  ...
  "resolutions": {
    "graphql-tools": "4.0.7"
  }
}

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