graphql-tools

graphql-tools copied to clipboard

The ResolversComposerMapping type is severely limited when attaching resolvers as nested properties property.field. It doesn't take custom resolvers and also doesn't check if the nested object path exists.

There's also a small bug when using a nested map with root objects that have all properties set as optional. Child objects will be treated as empty object literals skipping the excess property check and thus allowing the fields to be set to any name and type without any errors.

https://github.com/ardatan/graphql-tools/blob/681045468182ff249a22ea36d4dab65dc9a16464/packages/resolvers-composition/src/resolvers-composition.ts#L10-L22

I have a fairly good idea how to improve the typings to properly type check nested properties when using dot notation. With the introduction of template literal types in TS 4.1 this should be fairly straightforward task.

Fixing the bug for the nested map should also be fairly easy. Coercing the passed in resolvers map type to require all properties should do the trick.

Hi @Idono87 and thank you for the report!

I'm not adding a lot here but I'm just labeling it according to our new Contribution Guide and issue flow.

It looks like we are in stage 0.

Now in order to advance to stage 1 we'll need a reproduction, maybe create a sample project on Codesandbox? Then, in order to advance this to the next phase, maybe someone would be willing to contribute a failing test, demonstrating the wished output with the right configurations.

Thank you and sorry that this comment is not a complete solution (yet).

Create a sample project highlighting all issues and expected behavior at https://github.com/Idono87/graphql-tools-resolvers-composition-sample

Just do a quick npm run test after installing the required dependencies and it should hopefully work as intended by running the typescript compiler and not emit any files.

The following error should popup.

Code below hopefully explains what i'm expecting.

export const composedResolverMap: ResolversComposerMapping<{
  Mutation?: { login: () => null };
}> = {
  // Expecting TS compiler to throw an error when the Mutation object doesn't have an index signature
  'Mutation.this_property_should_not_exist': (next) => () => null,

  // Should not throw an error when using a resolver with custom args.
  'Mutation.login': (next) => (
    parent: any,
    args: { something: string },
    context: any,
    info: any
  ) => null,
};

export const composedResolverMap2: ResolversComposerMapping<{
  Mutation?: { login: () => null };
}> = {
  Mutation: {
    // Should throw an error when value is not a ResolversComposition value
    login: 'hello',

    // Should throw an error when property is not defined in the object type and
    // the object type does not have an index signature.
    not_defined: (next: any) => () => null,
  },
};

Thanks for the reproduction @ldono87 ! I am updating the label as stage-1. So in order to move on, we need to have a failing test and maybe even further like a local solution or more. We'd love to accept a draft PR if anyone is able to solve this.

@ardatan quick question. Is there a reason why strictNullChecks or strict isn't set in the tsconfig?

We never needed that much strictness in our codebase so far. That might be the reason :)

Ok. That does introduce some error prone typings for certain code.

Say for example there's a query that has a field that's not nullable.

const typeDefs = gql`
      type Query {
        foo(id: String!): String!
      }
`;

And an equivalent resolver with a return type that reflects the query.

const map: { [index: string]: string | undefined | null } = {
  "one": "foo",
  "two": "bar",
  "three": null,
}

function fooResolver(parent:any, args: {id: string}, context: any, info: any): string {
  return map[id];
}

So if we have something like the above code. The resolver looks through an indexable object to find a matching property field and returns the field value. If the property field does not exist, undefined will be returned, if the id being passed is "three" then null will be returned. Typescript will completely ignore the undefined and null types from the object index signature resulting in a runtime error by graphql for passing a undefined/null value when the value should be non nullable. This should generally caught by the transpiler if strictNullChecks is on since the index signature of the object does not match the resolver return type.

ResolverComposerMap is quite broken (and possibly other types to) because strictNullChecks is off. With strickNullChecks turned off we get code that allows assigning resolvers that return null and undefined to fields that should not.

interface Resolvers {
  Query?: {
    foo?: () => string
  }
}

export const rcMap: ResolversComposerMapping<Resolvers> = {
  Query: {
    foo: () => null
  }
}

If we have strictNullChecks turned on (like i tend to prefer) we get the behavior described in my above post. Where foo can be assigned anything. (edit: Query field has the same behavior. It can be set to anything resulting in the child object having any field with any type)

interface Resolvers {
  Query?: {
    foo: () => string
  }
}

export const rcMap: ResolversComposerMapping<Resolvers> = {
  Query: {
    foo: 'hello'
  }
}

I've got two options if you want me to follow the issue flow, especially stage 2.

1. Run tests with a separate tsconfig where the strictness is as tight as possible.
2. Tighten the strictness for the entire project.

I'm not a big fan of diverging configs for a library. So in my opinion option two would be the best with a few pros and cons of increased strictness.

• Largest pro, be better type security with better code suggestions.
• Largest con, the entire library will break with allot of failed type checks because of the increased strictness.

Just to summarize. In my opinion having less strictness in graphql-tools inadvertently decreases type security for projects with more strictness and also introduces unwanted behavior. The way to fix this is to increase strictness which will expose the unwanted behavior to be fixed and still keep the current behavior for less strict configurations.

@Idono87 I guess we just didn't do it for no really good reason. Maybe you can start by opening a draft PR with changing tsconfig to strict and we could all collaborate to gradually solve all issues?

Done. Just wanted to get some feedback on my thoughts.

thank you! let's continue there and gradually get it better!

It seems we already solved the missing strictness in TS types. Closing the issue then. Let us know if there are still any issues with TS strictness.