Duplicated nested fragments on code generation

[zaguiini]

Describe the bug

When the TypedDocumentNode is generated by the tool, it splats (...) all the fragment definitions, and then the nested fragments follow the same pattern, recursively, resulting in duplicated fragments being sent over the wire.

A lot of GraphQL APIs have limits regarding how many fragments are allowed per query, so this is also a problem.

To Reproduce

Here's a reproducible example of what's happening. Clone it, install and run yarn show-generated-content and it will output the definitions included in the typed node.

Expected behavior
Nested fragments are not duplicated on documents.

Environment:

Specified on the reproducible repo.

Additional context

graphql-tag workarounds it by removing the duplicated definitions. I don't think it's straightforward to achieve the same behavior on this plugin since it relies on static code generation instead of dynamic, but I do believe it's possible to achieve the same result.

In the meantime, I'm using a palliative solution that patches this plugin. Here's the diff:

diff --git a/node_modules/@graphql-codegen/typed-document-node/index.cjs.js b/node_modules/@graphql-codegen/typed-document-node/index.cjs.js
index b305f91..eeabaa8 100644
--- a/node_modules/@graphql-codegen/typed-document-node/index.cjs.js
+++ b/node_modules/@graphql-codegen/typed-document-node/index.cjs.js
@@ -8,6 +8,47 @@ const graphql = require('graphql');
 const path = require('path');
 const visitorPluginCommon = require('@graphql-codegen/visitor-plugin-common');
 const autoBind = _interopDefault(require('auto-bind'));
+const ts = require('typescript');
+
+const includeFragmentDeduplicator = operation => {
+    const tree = ts.createSourceFile(
+        'file',
+        operation,
+        undefined,
+        true
+    );
+
+    const transformer = context => root => {
+        const visit = (node) => {
+            node = ts.visitEachChild(node, visit, context);
+
+            if(node.kind === ts.SyntaxKind.ObjectLiteralExpression) {
+                const kindPropertyIndex = node.properties.findIndex(property => {
+                    return property.name && property.name.text === 'kind'
+                });
+
+                const hasKindProperty = kindPropertyIndex !== -1;
+                const isDocumentKind = () => node.properties[kindPropertyIndex].initializer.text === 'Document';
+
+                if(hasKindProperty && isDocumentKind()) {
+                    return ts.createCall(
+                        ts.createIdentifier('dedupeFragments'),
+                        undefined,
+                        [node]
+                    );
+                }
+            }
+
+            return node;
+        }
+
+        return ts.visitNode(root, visit);
+    }
+
+    const result = ts.transform(tree, [transformer]);
+
+    return ts.createPrinter().printFile(result.transformed[0]);
+}
 
 class TypeScriptDocumentNodesVisitor extends visitorPluginCommon.ClientSideBaseVisitor {
     constructor(schema, fragments, rawConfig, documents) {
@@ -47,9 +88,21 @@ const plugin = (schema, documents, config) => {
     ];
     const visitor = new TypeScriptDocumentNodesVisitor(schema, allFragments, config, documents);
     const visitorResult = graphql.visit(allAst, { leave: visitor });
+
+    const operations = visitorResult.definitions.filter(t => typeof t === 'string').map(includeFragmentDeduplicator);
+
+    const content = [visitor.fragments, ...operations].join('\n');
+
+    if(operations.length > 0) {
+        const dedupeImportName = visitor._parseImport('~lib/dedupeFragments#dedupeFragments');
+        const dedupeImport = visitor._generateImport(dedupeImportName, 'dedupeFragments', false);
+
+        visitor._imports.add(dedupeImport);
+    }
+
     return {
         prepend: visitor.getImports(),
-        content: [visitor.fragments, ...visitorResult.definitions.filter(t => typeof t === 'string')].join('\n'),
+        content: content,
     };
 };
 const validate = async (schema, documents, config, outputFile) => {

Basically, this wraps the operations with this dedupeFragments function:

import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core'
import { FragmentDefinitionNode } from 'graphql'

export const dedupeFragments = <Operation, Variables>(
  document: DocumentNode<Operation, Variables>
): DocumentNode<Operation, Variables> => {
  const seenFragments = new Set()

  return {
    ...document,
    definitions: document.definitions.filter(definition => {
      if (definition.kind === 'FragmentDefinition') {
        const fragmentDefinition = definition as FragmentDefinitionNode

        const fragmentName = fragmentDefinition.name.value

        if (seenFragments.has(fragmentName)) {
          return false
        }

        seenFragments.add(fragmentName)
      }

      return true
    })
  }
}

Which does the job of analyzing the operations and ensures that no repeated fragments are sent.

What can be done to address that? Do we have some direction?

[dotansimha]

@ardatan can you please take a look? :)

[dotansimha]

It took a look, and it seems like it might be a bit tricky to solve, since we define fragments in each operation (and fragments).
The configuration flag documentMode: DocumentMode.documentNodeImportFragments uses the fragments definition object, so we might not be able to solve cases like this:

fragment A on User {
  ...Nested
}

fragment B on User {
  ...Nested
}

query user {
   user {
      ...A
      ...B
      ...Nested
   }
}

Because even it we'll be able to avoid adding Nested field in the definition itself, A and B will add it, since it will be referenced directly from the variable, which will use spread there to get the fragment itself. So eventually, it will include Nested twice anyway.

I think we might not be able to solve that without a runtime method to filter the result DocumentNode.definitions. @ardatan what do you think?

[zaguiini]

One option would be to spin a TypeScript process that takes the file as input and produces the already filtered fragment output. Would it be possible?

[KnisterPeter]

Just for the reference: The graphql-tag/loader solves that with a runtime function as well:

https://github.com/apollographql/graphql-tag/blob/2b99a7c31a52a8e4927cc01c47bdc11e0539085f/loader.js#L9-L40

[bartenra]

We ended up with a 30MB GraphQL type definition file because of this. 🤣

[dotansimha]

@gilgardosh created a PR for fixing that during generation time: #6018 , by deduplicating the fragments and include them only at the level of the operation.

[dotansimha]

This issue is now fixed. To get this fix, make sure to update to the latest plugins (this change affects many plugins, see the list here: #6150 ), and make sure to add to your config: dedupeFragments: true.

The dedupe happens in generation time, by adding the fragments only once, to the operation, without using them on fragments.

Thanks @gilgardosh for fixing it :)

[SimenB]

Any particular reason this is not the default behaviour? After moving to typed-document-node we had to use this or @apollo/server rejected the request due to duplicated fragments.

And even if some runtime stripped/ignore them, deduping seems to have no real drawbacks and just reduces data sent over the wire and/or processed?

[tshddx]

Just to add some clarity, this dedupeFragments: true config option is available on several plugins. This includes typed-document-node, even though there are no config options mentioned in the docs for typed-document-node. It wasn't clear to me based on this thread where to add the config option, but here's what worked for me:

Before:

// Inside graphql.config.js

  plugins: [
    'typescript',
    'typescript-operations',
    'typed-document-node'
  ]

After:

// Inside graphql.config.js

  plugins: [
    'typescript',
    'typescript-operations',
    {
      'typed-document-node': {
        dedupeFragments: true,
      },
    },
  ]

[pleunv]

As far as I can tell, enabling dedupeFragments then does lead to the generated fragment files still containing imports of nested fragments that end up being unused, which, combined with TS's noUnusedLocals, causes type errors.