Duplicate IResolvers definitions breaks build

jbreeden-splunk commented 5 years ago

Description

Graphqlgen overrides the declaration of IResolvers from the graphql-tools package in the generated graphqlgen.ts file. This causes an error when you try to compose a schema from two packages that both use graphqlgen.

Each package will have a declaration like so:

// @ts-ignore
declare module 'graphql-tools' {
    interface IResolvers extends Resolvers {}
}

Where Resolvers is a unique type generated within the package itself.

Since IResolvers will be defined in both packages, with different definitions, you get the following error at build time:

TSError: ⨯ Unable to compile TypeScript:
src/generated/graphqlgen.ts(8464,15): error TS2320: Interface 'IResolvers' cannot simultaneously extend types 'Resolvers' and 'Resolvers'.
  Named property 'Mutation' of types 'Resolvers' and 'Resolvers' are not identical.
src/generated/graphqlgen.ts(8464,15): error TS2320: Interface 'IResolvers' cannot simultaneously extend types 'Resolvers' and 'Resolvers'.
  Named property 'Query' of types 'Resolvers' and 'Resolvers' are not identical.

Steps to reproduce

Create a graphql service, using graphqlgen
Create a package, also using graphqlgen, that exports an executable schema
- This package should be built with declaration files enabled
Import the package into your graphqlservice
Configure your service to mergeSchemas when starting your service
Try to build the service
You should get the error because each package now has a conflicting IResolvers definition

Note, I'm not actually even using these types. Their presence in graphqlgen.ts appears to be enough to cause the error.

Expected results

No error is thrown

Actual results

TSError: ⨯ Unable to compile TypeScript

Versions

graphqlgen: 0.6.0-rc9
OS name and version: OSX 10.14.3

jbreeden-splunk commented 5 years ago

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?

jasonkuhrt commented 5 years ago

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.

jbreeden-splunk commented 5 years ago

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 ¯\_(ツ)_/¯

jasonkuhrt commented 5 years ago

@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?

jbreeden-splunk commented 5 years ago

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.

jasonkuhrt commented 5 years ago

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.

jbreeden commented 5 years ago

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.

jbreeden-splunk commented 5 years ago

@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)

jasonkuhrt commented 5 years ago

@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)?

joe-re commented 5 years ago

@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
...
├─ graphqlgen@0.6.0-rc9
│  ├─ graphqlgen-json-schema@0.6.0-rc8

Maybe graphqlgen-json-schema has not released yet?

jasonkuhrt commented 5 years ago

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

prisma-labs / graphqlgen