Duplicate IResolvers definitions breaks build about graphqlgen HOT 11 OPEN

I'm able to work around this by backing up the graphqlgen dependency for one of the two packages involved to a version that doesn't output the graphql-tools declaration.

However, the IResolvers interface is unusable in the main service because the imported package still contains a declaration for it, which is not correct for the service.

It seems like the IResolvers declaration may have been a bit aggressive. What problem is it solving exactly? Is there perhaps another way to accomplish the same thing?

from graphqlgen.

Hey @jbreeden-splunk, the problem it addresses is #15

I don't have time to dive into the issue right now, but will try to loop back tonight.

Any ideas you have to possibly address this issue are welcome.

from graphqlgen.

Thanks for the quick response, @jasonkuhrt

If I've understood correctly, we're trying to allow graphqlgen resolvers to be compatible with graphql-tool's IResolvers interface, so that you can pass them to makeExecutableSchema and the like.

The problem with the chosen approach is that globally swapping the types of a 3rd party package with fundamentally incompatible types is always going to cause problems for larger apps. These apps are often composed of many packages making it impossible for graphqlgen to predict the full context under which the altered types are being used.

IMO a safer approach would simply be to make the types actually compatible. If the only problem is the missing index signature, we could simply add it.

I did see your note in #15:

The any-cast is a cheap workaround. I believe this is less evil than introducing index type into graphqlgen. Even semantically it’s rubberish. Resolvers object is not a black box index!

And I agree with the sentiment regarding the black box, but the fact is it is a black box to graphql-tools, and if you wish to be compatible I think it would be more appropriate to alter your own type definitions, rather than theirs.

If it's that much of a sticking point, I would recommend documenting the module declaration or type assertion workarounds, but not including them in the codegen with no way to disable it.

Hope that's somewhat helpful ¯\_(ツ)_/¯

from graphqlgen.

@jbreeden-splunk thanks for sharing your perspective, it was helpful!

I appreciate your point. In general, it is what I thought and think too.

In practice, I would be interested in seeing a project that needs the unsafe version of IResolvers. Anyways, this issue is not about that.

This causes an error when you try to compose a schema from two packages that both use graphqlgen.

To my knowledge this is a novel use-case. Thanks for bringing it to light. I would like to understand it better.

Is the consuming project of these two packages itself a graphqlgen project?

I believe one solution may be to make it so that by default graphqlgen does not generate its own specification for IResolvers. If a user wants that, they explicitly set in their config, for example:

compatibleIResolvers: yes

Imagining we had this solution for a moment, in your case @jbreeden-splunk, assuming your root project is a graphqlgen one (question above) would you enable the option in your project? If not, why?

from graphqlgen.

Is the consuming project of these two packages itself a graphqlgen project?

Yes, it is. Basically I have a monorepo setup like this:

monorepo/
    packages/
        graphql-module
    services/
        graphql

• graphql-module contains an executable schema for a portion of our schema, which is consumed by multiple applications, including the graphql service in this monorepo
• graphql service contains a builtin executable schema, but also imports other modules and merges the schema before starting the server.

Both of these packages use graphqlgen to generate resolver type definitions, so they both contain the graphql-tools declaration in graphqlgen.ts.

I believe one solution may be to make it so that by default graphqlgen does not generate its own specification for IResolvers

That would work for me. If the option was there, I would probably leave it disabled for all packages and just perform the requisite type assertion when passing the resolvers to graphql-tools. Even in the root package, my merged resolvers object is not going to conform to the generated Resolvers interface, so I don't think it would be of any use.

from graphqlgen.

That would work for me

Good to know, I think this is a reasonable approach for this project to take presently.

I also believe it should be opt-in, not opt-out, due to the leaky abstraction we both agree it represents.

I would probably leave it disabled for all packages and just perform the requisite type assertion when passing the resolvers to graphql-tools.

👍

Even in the root package

Out of curiosity and trying to understand your use-case better;

• Does the root project introduce its own resolvers or its simply a pure merge of the sub-packges?
• How does the root project manage its GQL SDL?

I will aim to have a fix for this merged and released by next Sunday.

Pull-requests welcome in the meantime.

from graphqlgen.

Awesome, thanks for the help!

Does the root project introduce its own resolvers or its simply a pure merge of the sub-packages?

It has its own resolvers, in addition to merging in schema from sub-packages.

How does the root project manage its GQL SDL?

Right now it's just one big SDL file. We're looking at switching to resolver-first development with Nexus, but haven't yet.

from graphqlgen.

@jasonkuhrt Just noticed the iresolvers-augmentation field in the docs: https://oss.prisma.io/graphqlgen/01-configuration.html

I tried to disable it, but graphqlgen yells at me saying it's unrecognized:

yarn run v1.13.0
$ graphqlgen
Invalid graphqlgen.yml file
 should NOT have additional properties. additionalProperty: iresolvers-augmentation
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Even though it is definitely present (in some form) in my graphqlgen package:

ack iResolversAugmentationEnabled
types.d.ts
12:    iResolversAugmentationEnabled: boolean;

index.js
108:        iResolversAugmentationEnabled: typeof codeGenArgs.config['iresolvers-augmentation'] === 'boolean'

generators/typescript/generator.js
54:    return "  " + renderHeader(args, { hasPolymorphicObjects: hasPolymorphicObjects }) + "\n\n  " + common_1.renderEnums(args) + "\n\n  " + renderNamespaces(args, interfacesMap, unionsMap, typeToInputTypeAssociation, inputTypesMap) + "\n\n  " + renderResolvers(args) + "\n\n  " + (args.iResolversAugmentationEnabled

It seems it's in there, but not fully implemented. Am I doing something wrong or are the docs just ahead of the actual release?

(Tested on version 0.6.0-rc9)

from graphqlgen.

@jbreeden-splunk 🤦‍♂️ right

I think that error comes from graphqlgen-json-schema?

Its updated though https://github.com/prisma/graphqlgen/search?utf8=%E2%9C%93&q=iresolvers-augmentation&type=

Are you using the latest version of it too (0.6.0-rc8)?

from graphqlgen.

@jasonkuhrt
Hi I checked my schema.json file on graohql-json-schema under node_modules and seems like it doen't include iresolvers-augumentation propery.

{
  "$schema": "http://json-schema.org/draft-06/schema#",
  "title": "JSON schema for graphqlgen.yml files",
  "properties": {
    "language": {
      "type": "string",
      "oneOf": [{ "enum": ["typescript", "flow"] }]
    },
    "schema": {
      "type": "string"
    },
    "context": {
      "type": "string"
    },
    "models": {
      "type": "object",
      "properties": {
        "files": {
          "type": "array",
          "items": {
            "anyOf": [
              {
                "type": "string"
              },
              {
                "type": "object",
                "properties": {
                  "path": {
                    "type": "string"
                  },
                  "defaultName": {
                    "type": "string"
                  }
                },
                "required": ["path"]
              }
            ]
          }
        },
        "override": {
          "type": "object",
          "patternProperties": {
            "^[a-zA-Z0-9]*$": {
              "type": "string"
            }
          }
        }
      },
      "additionalProperties": false
    },
    "output": {
      "type": "string",
      "description": "Path to main output file"
    },
    "resolver-scaffolding": {
      "description": "All output fields",
      "type": "object",
      "properties": {
        "output": {
          "type": "string",
          "description": "Path to scaffolded file for missing model definitions"
        },
        "layout": {
          "type": "string",
          "oneOf": [{ "enum": ["file-per-type"] }]
        }
      },
      "required": ["output", "layout"],
      "additionalProperties": false
    }
  },
  "required": ["language", "schema", "models", "output"],
  "additionalProperties": false
}

Version is here.

$ yarn list
...
├─ [email protected]
│  ├─ [email protected]

Maybe graphqlgen-json-schema has not released yet?

from graphqlgen.

Hey @joe-re just fyi this project isn't maintained anymore by me. I am working on nexus oriented tools now.

from graphqlgen.