mergeTypeDefs does not preserve repeated directives

[darrellwarde]

Issue workflow progress

Progress of the issue based on the Contributor Workflow

• 1. The issue provides a reproduction available on Github, Stackblitz or CodeSandbox

  Make sure to fork this template and run yarn generate in the terminal.

  Please make sure the GraphQL Tools package versions under package.json matches yours.

• 2. A failing test has been provided
• 3. A local solution has been provided
• 4. A pull request is pending review

---

Describe the bug

When merging type definitions using mergeTypeDefs, multiple of the same directive on a type are not preserved - only the last takes precedence.

To Reproduce

Steps to reproduce the behavior:

Run the following code:

const { mergeTypeDefs } = require("@graphql-tools/merge");
const { print } = require("graphql");
const { gql } = require("graphql-tag");

const typeDefs = gql`
  type User @key(fields: "id") {
    id: ID!
  }

  extend type User @key(fields: "name") {
    name: String!
  }
`;

const mergedTypeDefs = mergeTypeDefs(typeDefs);

console.log(print(mergedTypeDefs));

The following type definitions are printed in the console:

type User @key(fields: "name") {
  id: ID!
  name: String!
}

Expected behavior

I would expect the output to be:

type User @key(fields: "id") @key(fields: "name") {
  id: ID!
  name: String!
}

Environment:

• OS: macOS 12.5.1
• @graphql-tools/merge: 8.3.6
• NodeJS: 16.13.0

Additional context

I have another observation around the argument convertExtensions - it sounds to me like if set to false, this would preserve extensions in the type definitions. However, if running the following code (same as above with added option):

const { mergeTypeDefs } = require("@graphql-tools/merge");
const { print } = require("graphql");
const { gql } = require("graphql-tag");

const typeDefs = gql`
  type User @key(fields: "id") {
    id: ID!
  }

  extend type User @key(fields: "name") {
    name: String!
  }
`;

const mergedTypeDefs = mergeTypeDefs(typeDefs, { convertExtensions: false });

console.log(print(mergedTypeDefs));

The output is identical to above, when I would expect it to be:

type User @key(fields: "id") {
  id: ID!
}

extend type User @key(fields: "name") {
  name: String!
}

I realise this is a different issue, but figured it was worth mentioning here!

[ardatan]

Thanks for creating this issue! If anyone is interested, PRs are welcome!

[darrellwarde]

Investigating this now, and hoping you can validate my thinking.

Taking the test "should append and extend directives" from merge-typedefs.spec.ts, and the following input:

const merged = mergeTypeDefs([
        `directive @id(primitiveArg: String, arrayArg: [String]) on FIELD_DEFINITION`,
        `type MyType { id: Int }`,
        `type MyType { id: Int @id }`,
        `type MyType { id: Int @id(primitiveArg: "1") }`,
        `type MyType { id: Int @id(primitiveArg: "1", arrayArg: ["1"]) }`,
        `type MyType { id: Int @id(arrayArg: ["2"]) }`,
        `type Query { f1: MyType }`,
      ]);

This results in output:

type MyType {
    id: Int @id(primitiveArg: "1", arrayArg: ["1", "2"])
}

I think this makes sense given that they are all object definitions. This gets tricky when it comes to object extensions, say if we just make one of the definitions above an extension:

const merged = mergeTypeDefs([
        `directive @id(primitiveArg: String, arrayArg: [String]) on FIELD_DEFINITION`,
        `type MyType { id: Int }`,
        `type MyType { id: Int @id }`,
        `type MyType { id: Int @id(primitiveArg: "1") }`,
        `type MyType { id: Int @id(primitiveArg: "1", arrayArg: ["1"]) }`,
        `extend type MyType { id: Int @id(arrayArg: ["2"]) }`,
        `type Query { f1: MyType }`,
      ]);

What should the output be here? In my opinion, this should result in the following output:

type MyType {
    id: Int @id(primitiveArg: "1", arrayArg: ["1"]) @id(arrayArg: ["2"])
}

Essentially, evaluating the object type definitions first, merging the arguments, and then evaluating the object extensions, adding them as separate directives.

I would appreciate your thoughts!

[wKich]

I faced with the same problem. @darrellwarde I think we shouldn't do deep merge arguments. And I guess current behaviour treats that every directive isn't repeatable. So as possible solution, I suggest we should check if a directive is repeatable and add it to result. But if directive isn't repeatable we should override old directive arguments

@ardatan, what do you think? I can take it to do a PR

[wKich]

@ardatan @darrellwarde I've made a PR for this issue. Could you please review it #5216

[ardatan]

Thanks for the PR! Merged it!