Improve directive and fragment removal logic

.changeset/dry-radios-help.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Keep `__typename` fragment when it does not contain `@client` directive and strip out inline fragments which use a `@client` directive. Thanks @Gazler and @mtsmfm!

config/bundlesize.ts

@@ -3,7 +3,7 @@ import { join } from "path";
 import { gzipSync } from "zlib";
 import bytes from "bytes";
 
-const gzipBundleByteLengthLimit = bytes("32.42KB");
+const gzipBundleByteLengthLimit = bytes("32.49KB");
 const minFile = join("dist", "apollo-client.min.cjs");
 const minPath = join(__dirname, "..", minFile);
 const gzipByteLen = gzipSync(readFileSync(minPath)).byteLength;

src/__tests__/local-state/resolvers.ts

@@ -218,10 +218,6 @@ describe('Basic resolver capabilities', () => {
     const serverQuery = gql`
       fragment Foo on Foo {
         bar
-        ...Foo2
-      }
-      fragment Foo2 on Foo {
-        __typename
       }
       query Mixed {
         foo {

src/utilities/graphql/__tests__/transform.ts

@@ -102,6 +102,39 @@ describe('removeFragmentSpreadFromDocument', () => {
   });
 });
 describe('removeDirectivesFromDocument', () => {
+ it('should remove inline fragments using a directive', () => {
+    const query = gql`
+      query Simple {
+        networkField
+        field {
+          ... on TypeA {
+            typeAThing
+          }
+          ... on TypeB @client {
+            typeBThing @client
+          }
+        }
+      }
+    `;
+
+    const expected = gql`
+      query Simple {
+        networkField
+        field {
+          ... on TypeA {
+            typeAThing
+          }
+        }
+      }
+    `;
+
+    const doc = removeDirectivesFromDocument(
+      [{ name: 'client', remove: true }],
+      query,
+    )!;
+    expect(print(doc)).toBe(print(expected));
+  });
+
   it('should not remove unused variable definitions unless the field is removed', () => {
     const query = gql`
       query Simple($variable: String!) {
@@ -948,4 +981,120 @@ describe('removeClientSetsFromDocument', () => {
     const doc = removeClientSetsFromDocument(query)!;
     expect(print(doc)).toBe(print(expected));
   });
+
+  it("should remove @client and __typename only fragment when query precedes fragment", () => {
+    const query = gql`
+      query {
+        author {
+          name
+          ...toBeRemoved
+        }
+      }
+
+      fragment toBeRemoved on Author {
+        __typename
+        isLoggedIn @client
+      }
+    `;
+
+    const expected = gql`
+      query {
+        author {
+          name
+        }
+      }
+    `;
+
+    const doc = removeClientSetsFromDocument(query)!;
+    expect(print(doc)).toBe(print(expected));
+  });
+
+  // TODO(FIXME): https://github.com/apollographql/apollo-client/issues/10539
+  it.skip("should remove @client and __typename only fragment when fragment precedes query", () => {
+    const query = gql`
+      fragment toBeRemoved on Author {
+        __typename
+        isLoggedIn @client
+      }
+
+      query {
+        author {
+          name
+          ...toBeRemoved
+        }
+      }
+    `;
+
+    const expected = gql`
+      query {
+        author {
+          name
+        }
+      }
+    `;
+
+    const doc = removeClientSetsFromDocument(query)!;
+    expect(print(doc)).toBe(print(expected));
+  });
+
+  it("should not remove __typename only fragment (without @client) when query precedes fragment", () => {
+    const query = gql`
+      query {
+        author {
+          name
+          ...authorInfo
+        }
+      }
+
+      fragment authorInfo on Author {
+        __typename
+      }
+    `;
+
+    const expected = gql`
+      query {
+        author {
+          name
+          ...authorInfo
+        }
+      }
+
+      fragment authorInfo on Author {
+        __typename
+      }
+    `;
+
+    const doc = removeClientSetsFromDocument(query)!;
+    expect(print(doc)).toBe(print(expected));
+  });
+
+  it("should not remove __typename only fragment (without @client) when fragment precedes query", () => {
+    const query = gql`
+      fragment authorInfo on Author {
+        __typename
+      }
+
+      query {
+        author {
+          name
+          ...authorInfo
+        }
+      }
+    `;
+
+    const expected = gql`
+      fragment authorInfo on Author {
+        __typename
+      }
+      query {
+        author {
+          name
+          ...authorInfo
+        }
+      }
+    `;
+
+    const doc = removeClientSetsFromDocument(query)!;
+    expect(print(doc)).toBe(print(expected));
+  });
 });

src/utilities/graphql/transform.ts

@@ -95,6 +95,21 @@ function getDirectiveMatcher(
   };
 }
 
+function shouldRemoveField(
+  directives: RemoveDirectiveConfig[],
+  nodeDirectives: FieldNode["directives"]
+) {
+  if (!nodeDirectives) return false;
+  const hasRemoveDirective = directives.some(directive => directive.remove);
+  if (
+    hasRemoveDirective &&
+    nodeDirectives.some(getDirectiveMatcher(directives))
+  ) {
+    return true;
+  }
+  return false;
+}
+
 export function removeDirectivesFromDocument(
   directives: RemoveDirectiveConfig[],
   doc: DocumentNode,
@@ -122,51 +137,74 @@ export function removeDirectivesFromDocument(
         },
       },
 
-      Field: {
+      FragmentDefinition: {
         enter(node) {
-          if (directives && node.directives) {
-            // If `remove` is set to true for a directive, and a directive match
-            // is found for a field, remove the field as well.
-            const shouldRemoveField = directives.some(
-              directive => directive.remove,
+          if (node.selectionSet) {
+            const isTypenameOnly = node.selectionSet.selections.every(
+              (selection) => isField(selection) &&
+                selection.name.value === "__typename"
             );
-
-            if (
-              shouldRemoveField &&
-              node.directives &&
-              node.directives.some(getDirectiveMatcher(directives))
-            ) {
-              if (node.arguments) {
-                // Store field argument variables so they can be removed
-                // from the operation definition.
-                node.arguments.forEach(arg => {
-                  if (arg.value.kind === 'Variable') {
-                    variablesToRemove.push({
-                      name: (arg.value as VariableNode).name.value,
-                    });
-                  }
-                });
-              }
-
-              if (node.selectionSet) {
-                // Store fragment spread names so they can be removed from the
-                // document.
-                getAllFragmentSpreadsFromSelectionSet(node.selectionSet).forEach(
-                  frag => {
-                    fragmentSpreadsToRemove.push({
-                      name: frag.name.value,
-                    });
-                  },
-                );
-              }
-
-              // Remove the field.
+            const isTypenameAndRemovableDirectivesOnly =
+              node.selectionSet.selections.every(
+                (selection) => (
+                  isField(selection) &&
+                  selection.name.value === "__typename"
+                ) || (
+                    isField(selection) &&
+                    shouldRemoveField(directives, selection.directives)
+                  )
+              );
+
+            // If a fragment contains only a __typename field,
+            // do not remove it from the document.
+            if (isTypenameOnly) return;
+
+            // If a fragment consists only of a __typename field and field(s)
+            // annotated with the directive being removed, remove the fragment
+            // definition and add the fragment name to fragmentSpreadsInUse
+            // for removal.
+            if (isTypenameAndRemovableDirectivesOnly) {
+              fragmentSpreadsInUse[node.name.value] = false;
+              fragmentSpreadsToRemove.push({
+                name: node.name.value,
+              });
               return null;
             }
           }
         },
       },
 
+      Field: {
+        enter(node) {
+          if (shouldRemoveField(directives, node.directives)) {
+            if (node.arguments) {
+              // Store field argument variables so they can be removed
+              // from the operation definition.
+              node.arguments.forEach(arg => {
+                if (arg.value.kind === 'Variable') {
+                  variablesToRemove.push({
+                    name: (arg.value as VariableNode).name.value,
+                  });
+                }
+              });
+            }
+            if (node.selectionSet) {
+              // Store fragment spread names so they can be removed from the
+              // document.
+              getAllFragmentSpreadsFromSelectionSet(node.selectionSet).forEach(
+                frag => {
+                  fragmentSpreadsToRemove.push({
+                    name: frag.name.value,
+                  });
+                },
+              );
+            }
+            // Remove the field.
+            return null;
+          }
+        },
+      },
+
       FragmentSpread: {
         enter(node) {
           // Keep track of referenced fragment spreads. This is used to
@@ -175,6 +213,14 @@ export function removeDirectivesFromDocument(
         },
       },
 
+      InlineFragment: {
+        enter(node) {
+          if (shouldRemoveField(directives, node.directives)) {
+            return null;
+          }
+        },
+      },
+
       Directive: {
         enter(node) {
           // If a matching directive is found, remove it.
@@ -491,27 +537,5 @@ export function removeClientSetsFromDocument(
     document,
   );
 
-  // After a fragment definition has had its @client related document
-  // sets removed, if the only field it has left is a __typename field,
-  // remove the entire fragment operation to prevent it from being fired
-  // on the server.
-  if (modifiedDoc) {
-    modifiedDoc = visit(modifiedDoc, {
-      FragmentDefinition: {
-        enter(node) {
-          if (node.selectionSet) {
-            const isTypenameOnly = node.selectionSet.selections.every(
-              selection =>
-                isField(selection) && selection.name.value === '__typename',
-            );
-            if (isTypenameOnly) {
-              return null;
-            }
-          }
-        },
-      },
-    });
-  }
-
   return modifiedDoc;
 }