Introduce the `removeTypenameFromVariables` link to remove `__typename` from variables #10853

jerelmiller · 2023-05-10T03:06:20Z

The removal of __typename from variables introduced in #10724 was determined to be too much of a breaking change as JSON-scalars could be utilizing __typename without violating the GraphQL spec (more information on this discussion can be found in #10767).

This PR moves this behavior into a new link called removeTypenameFromVariables that can be added to any link chain. This has the advantage that it can be used by any terminating link, unlike #10724 where this functionality was limited to HttpLink, BatchHttpLink, and GraphQLWsLink.

By default, this link will remove all __typename keys from variables. If a user has a JSON-scalar that utilizes __typename, the scalar can be configured with the link to keep __typename in those fields. JSON scalar fields nested inside input objects can also be configured to ensure __typename is kept at those paths. NOTE: JSON scalars don't literally have to be called JSON. These scalars can show up with other names (e.g. Raw, Config, etc).

The API introduced in this PR was created with code generation in mind. Much like possibleTypes, we should be able to recommend a code generation approach that will extract the configuration for you.

The API is as follows:

import { removeTypenameFromVariables, KEEP } from '@apollo/client/link/remove-typename';

// With no configuration, all __typenames are removed by default
const link = removeTypenameFromVariables();

const link = removeTypenameFromVariables({
  except: {
    // Keep `__typename` on any variable that is declared as a `JSON` type
    JSON: KEEP,

    // Input objects may have JSON scalar fields. These can configured using an object declaration.

    // Keep __typename for `bar` and `baz` fields on any variable declared as a `FooInput` type
    FooInput: {
      bar: KEEP,
      baz: KEEP,
    },

    // Configure deeply nested fields for complex objects
    BarInput: {
      baz: {
        qux: KEEP
      }
    },

    // JSON scalars may show up at varying nesting levels.
    // This keeps `__typename` on `bar.baz` and `bar.qux.foo`
    FooInput: {
      bar: {
        baz: KEEP, 
        qux: {
          foo: KEEP
        }
      }
    },

    // Array fields are configured like objects. 
    // Keep all `__typename`s on the `config` field for each `widget`
    DashboardInput: {
      widgets: {
        config: KEEP
      }
    }
  }
});

These types are detected from the variable declarations in the query.

query FooQuery(
  # `foo` is detected as a `JSON` type
  $foo: JSON, 

  # `bar` is detected as a `BarInput` type
  # scalar fields on `BarInput` can be configured as discussed above
  $bar: BarInput
) {
  someField(foo: $foo, bar: $bar)
}

Checklist:

If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests

… types

…ars option

…k introduced in #10724

changeset-bot · 2023-05-10T03:06:24Z

🦋 Changeset detected

Latest commit: b94fbda

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jerelmiller · 2023-05-10T03:08:25Z

src/link/remove-typename/removeTypenameFromVariables.ts

+  excludeScalars?: (string | ScalarConfig)[];
+}
+
+export function removeTypenameFromVariables(


We've been using the term "strip typename", but I felt "remove typename" felt a bit more "professional". Let me know what you think! I'm happy to rename this back to stripTypenameFromVariables if we prefer that verbiage.

src/link/remove-typename/__tests__/removeTypenameFromVariables.ts

jerelmiller · 2023-05-10T03:24:06Z

src/utilities/common/omitDeep.ts

@@ -1,14 +1,28 @@
 import { DeepOmit } from '../types/DeepOmit';
 import { isPlainObject } from './objects';

-export function omitDeep<T, K extends string>(value: T, key: K) {
-  return __omitDeep(value, key);
+const BREAK: unique symbol = Symbol('BREAK');


I wonder if naming this something like KEEP_ALL would be more semantically meaningful. Would love to hear thoughts on whether BREAK makes sense as a name here or not. This is meant to mean "stop traversal and keep the omitted field for all values inside this object"

"break" as in the sense of "early return for this subtree" makes sense for me.

Here's some precedent for BREAK:
https://github.com/graphql/graphql-js/blob/d75872f5e64eb3ad212521b1054d8a1d298a8a89/src/language/visitor.ts#L87

Thats where I got the name from initially 😆

src/link/remove-typename/removeTypenameFromVariables.ts

phryneas · 2023-05-10T10:21:47Z

src/link/remove-typename/__tests__/removeTypenameFromVariables.ts

+  const link = removeTypenameFromVariables({
+    excludeScalars: [
+      {
+        scalar: 'JSON',


Wait... in this test, nothing mentions JSON anywhere. Can you explain what exactly the scalar property here does?

The paths describe all the fields where JSON scalar live. For example, if your schema with an input object looks like this:

scalar JSON type Query { someField(foo: FooInput) } input FooInput { bar: JSON baz: JSON }

All we can detect from the query is that foo is of type FooInput, but without full schema introspection, its difficult to know which fields on input objects are of type JSON, hence the paths option.

This scalar declaration declares JSON as a type that should be excluded when removing the typename. While this test doesn't include it, this means variables of type JSON should be excluded when removing the typename:

query Test($foo: JSON) { someField(foo: $foo) }

Does this help?

I'll add one more clarifying example that might hammer this home. Let's say my query looks like this:

query TestQuery($foo: FooInput, $json: JSON) { someField(foo: $foo, json: $json) }

With configuration:

removeTypenameFromVariables({ excludeScalars: [ { scalar: 'JSON', paths: { FooInput: ['bar'] } } ] });

This would keep __typename on variable json since its of type JSON and it would also keep __typename on the field bar of variable foo since its of type FooInput with a JSON scalar field bar.

Since excludeScalars is an array, this allows you to define several scalar types that should keep the __typename value:

removeTypenameFromVariables({ excludeScalars: [ { scalar: 'JSON', paths: { FooInput: ['bar'] }, { scalar: 'Raw' }, { scalar: 'Config' } ] }); // or you can use the string shorthand for scalar configs that don't have a `paths` option removeTypenameFromVariables({ excludeScalars: [ 'Raw', 'Config', { scalar: 'JSON', paths: { FooInput: ['bar'] }, ] });

Hopefully that helps give some more context

@phryneas I changed the API signature after some feedback. I've updated the PR description to reflect the change. Let me know what you think about the change!

src/link/remove-typename/removeTypenameFromVariables.ts

jerelmiller · 2023-05-10T21:34:47Z

After the initial review with @phryneas and talking this over a bit more with @benjamn, the API proposed in this PR seems a bit too confusing. It's not clear enough the effect the scalar configuration has grouped with the path configuration.

Instead, I am going to experiment with a fully object-based configuration. Scalars are just types that happens to live at the leaves of the tree, so the configuration doesn't have to differentiate itself from other non-scalar types with path configurations. The configuration might look something like this:

removeTypenameFromVariables({
  exclude: {
    JSON: true,
    FooInput: {
      bar: true,
      deeply: {
        nested: true
      }
    }
  }
})

This also has the advantage that it looks a little more like GraphQL queries themselves.

The trick here is figuring out the leaf delimiter. While true works well here and is plenty readable, one might surmise that you can also pass a false, which is not a valid value. Instead, we could consider exporting a special (serializable) identifier that denotes the leaf nodes. The advantage here is that it disambiguates what values are allowed at leaf fields. This might look something like:

// import name TBD
import { removeTypenameFromVariables, KEEP } from '@apollo/client/link/remove-typename';

removeTypenameFromVariables({
  exclude: {
    JSON: KEEP,
    FooInput: {
      bar: KEEP,
      deeply: {
        nested: KEEP
      }
    }
  }
})

where KEEP identifies types/fields that should be left alone. Here, all variables declared as type JSON would keep their __typename fields intact. Variables of type FooInput would keep __typename within their bar and deeply.nested fields.

…nfig form

benjamn · 2023-05-10T22:45:18Z

src/link/remove-typename/__tests__/removeTypenameFromVariables.ts

+  const link = removeTypenameFromVariables({
+    except: {
+      FooInput: {
+        bar: KEEP,
+        baz: {
+          qux: KEEP,
+        },
+      },
+    },
+  });


So readable!

src/utilities/common/stripTypename.ts

benjamn · 2023-05-10T23:23:47Z

src/link/remove-typename/removeTypenameFromVariables.ts

+      variables: stripTypename(variables, {
+        keep: (variablePath) => {
+          const typename = variableDefinitions[variablePath[0]];
+
+          // The path configurations do not include array indexes, so we
+          // omit them when checking the `trie` for a configured path
+          const withoutArrayIndexes = variablePath.filter(
+            (segment) => typeof segment === 'string'
+          );
+
+          // Our path configurations use the typename as the root so we need to
+          // replace the first segment in the variable path with the typename
+          // instead of the top-level variable name.
+          return trie.peekArray([typename, ...withoutArrayIndexes.slice(1)]);


How essential is this options.keep callback API for stripTypename and omitDeep? If there was another way to implement this code, would we still want to ship options.keep?

Its essential in the fact that we need to know when to leave the key at a certain path alone vs remove it, but the callback is more an implementation detail than a necessity. I had considered an object-based approach, but couldn't figure out a good way to represent the syntax for array items.

Since omitDeep is designed to be a general purpose utility, I could see cases where we might want to use it in some way to iterate over data from an interface type, where we conditionally omit a field in that union type based on the concrete type (though I have no use case for something like this at the moment). The callback style API felt the most flexible way to allow for everything.

I'm curious, what do you have in mind? I'm all ears and not attached to this if you have a better idea in mind.

My hypothetical concern was that we might end up calling keep for every possible path in a very large variables object, which could be expensive since the keep function is creating three new arrays in addition to the Trie lookup.

Instead, we might be able to call stripTypename on just the subtrees that are not protected by KEEP, without needing to configure the stripTypename call with a keep callback. In other words, we'd be calling stripTypename multiple times on different subtrees of the object, and since each of those calls strips all __typename fields in its subtree, we already know what keep would return for that entire subtree, so calling keep becomes unnecessary.

On the other hand, if/when we come up with something more efficient, we should be able to switch over to a different implementation without changing the behavior of this code. In that scenario, we might not end up using keep internally, but I don't have a problem with the keep API existing as part of stripTypename and omitDeep.

In other words, this question doesn't need to block the PR, but I will keep thinking about it, in case a good alternative occurs to me.

Ya this is a great observation. Totally agree, let's keep noodling on this. If we can find a good way to implement this, I'd be all for this optimization.

I'll go ahead and merge this PR as-is and once we have a good optimization solution, we can open up a follow-up refactor PR. I would consider these helpers internal so we can make breaking changes to the function signature if/when we need to.

Thanks for thinking this through!

phryneas · 2023-05-11T12:16:48Z

The configuration might look something like this:
removeTypenameFromVariables({
  exclude: {
    JSON: true,
    FooInput: {
      bar: true,
      deeply: {
        nested: true
      }
    }
  }
})
This also has the advantage that it looks a little more like GraphQL queries themselves.

In this example, how can I exclude the typename on FooInput from being removed, but have it removed in some of it's children?

jerelmiller · 2023-05-11T17:23:33Z

@phryneas the configuration is meant to represent the fields on types that have JSON scalar as their types. FooInput here is meant to represent an input object type in the schema, not a JSON scalar itself. In other words that example represents a schema that looks like this:

scalar JSON

type Query {
  someField(foo: FooInput)
}

input FooInput {
  bar: JSON
  baz: BazInput
}

input BazInput {
  deeply: DeeplyInput
}

input DeeplyInput {
  nested: JSON
}

Leaving __typename on FooInput in this case would be a violation of the GraphQL spec since FooInput is an input type, not a JSON scalar. The only way leaving __typename on FooInput makes sense is if FooInput is a scalar itself, but the configuration should be FooInput: KEEP in that case.

The configuration was designed with input types in mind where you could tell us which fields are the JSON scalar types since we can't determine that from the query itself. In that way you should only be able to configure a KEEP at a leaf field, not within something that has a subselection.

Does that make sense?

phryneas · 2023-05-15T08:26:38Z

Yeah, with that explanation it does. I get a feeling that this one will need a very good example in the docs, though.

jerelmiller · 2023-05-15T16:27:33Z

I get a feeling that this one will need a very good example in the docs, though.

Oh absolutely. I'm willing to bet this is a very very VERY small percentage of users that will even care about keeping __typename on json scalars to begin with. I'm willing to bet 99.9% of usage will be to simply strip __typename from everything.

.changeset/silent-eagles-check.md

…e-link

github-actions · 2023-05-15T23:35:06Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	35.8 KB (-0.49% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	45.26 KB (+0.25% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	34.05 KB (-0.42% 🔽)
import { ApolloProvider } from "dist/react/index.js"	867 B (0%)
import { useQuery } from "dist/react/index.js"	25.42 KB (-0.59% 🔽)
import { useLazyQuery } from "dist/react/index.js"	25.75 KB (-0.62% 🔽)
import { useMutation } from "dist/react/index.js"	23.95 KB (-0.62% 🔽)
import { useSubscription } from "dist/react/index.js"	2.36 KB (0%)
import { useSuspenseQuery_experimental } from "dist/react/index.js"	3.53 KB (0%)
import { useBackgroundQuery_experimental } from "dist/react/index.js"	2.97 KB (0%)
import { useReadQuery_experimental } from "dist/react/index.js"	1.12 KB (0%)
import { useFragment_experimental } from "dist/react/index.js"	1.78 KB (0%)

jerelmiller added 16 commits May 8, 2023 20:13

Add option to omitDeep to allow some paths to be kept

bf297c7

Allow typename to be kept with keep option

c8db491

Create removeTypenameFromVariables link that handles excluding scalar…

84a5f94

… types

Add excludeScalarPaths to removeTypenameFromVariables options

62bd9e3

Combine excludeScalars and excludeScalarPaths into single excludeScal…

2a02f4b

…ars option

Small refactor to destructure variables from result

6e86608

Minor tweaks to test names

9c990ca

Add additional test to ensure mixed forms work well

269cb55

Refactor collectPaths for clarity and move out of link

d0fb068

Add clarifying comments

df99d13

Export RemoveTypenameFromVariablesOptions

7b53b02

Add remove-typename link as an entry point

d0f85b9

Undo automatic stripping of __typename from HttpLink and GraphQLWsLin…

04896a2

…k introduced in #10724

Add additional test for keep option to stripTypename tests

0065bc5

Update exports test to include new link

c5c1a53

Add changeset

717c4e4

jerelmiller requested review from benjamn, phryneas and alessbell May 10, 2023 03:06

jerelmiller commented May 10, 2023

View reviewed changes

src/link/remove-typename/__tests__/removeTypenameFromVariables.ts Outdated Show resolved Hide resolved

Minor refactor to simplify collectPaths

3167fd9

jerelmiller commented May 10, 2023

View reviewed changes

jerelmiller changed the title ~~Introduce the removeTypenameFromVariables link to allow __typename to be stripped from variables~~ Introduce the removeTypenameFromVariables link to remove __typename from variables May 10, 2023

Merge branch 'release-3.8' into remove-typename-link

2f39292

phryneas reviewed May 10, 2023

View reviewed changes

jerelmiller commented May 10, 2023

View reviewed changes

src/link/remove-typename/removeTypenameFromVariables.ts Outdated Show resolved Hide resolved

Rename excludeScalars to except and switch to new object-based co…

ab9d458

…nfig form

jerelmiller added 2 commits May 10, 2023 16:32

Minor update to comment

16e9300

Update snapshot tests

33dd969

benjamn approved these changes May 10, 2023

View reviewed changes

jerelmiller commented May 15, 2023

View reviewed changes

.changeset/silent-eagles-check.md Outdated Show resolved Hide resolved

jerelmiller added 2 commits May 15, 2023 12:42

Switch changeset change from patch to minor

2a80a40

Merge remote-tracking branch 'origin/release-3.8' into remove-typenam…

70b8693

…e-link

Fix lint errors

b94fbda

jerelmiller merged commit 3009579 into release-3.8 May 16, 2023

jerelmiller deleted the remove-typename-link branch May 16, 2023 00:13

github-actions bot mentioned this pull request May 16, 2023

Version Packages (alpha) #10835

Merged

This was referenced May 16, 2023

Auto strip __typename does not correctly handle JSON scalars #10767

Closed

More efficient implementation when conditionally stripping typename from variables #10890

Merged

jerelmiller mentioned this pull request May 26, 2023

Ensure operation.getContext and operation.setContext functions are callable for links after removeTypenameFromVariables #10919

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the `removeTypenameFromVariables` link to remove `__typename` from variables #10853

Introduce the `removeTypenameFromVariables` link to remove `__typename` from variables #10853

jerelmiller commented May 10, 2023 •

edited

Loading

changeset-bot bot commented May 10, 2023 •

edited

Loading

jerelmiller May 10, 2023

jerelmiller May 10, 2023

phryneas May 10, 2023

benjamn May 10, 2023

jerelmiller May 10, 2023

phryneas May 10, 2023

jerelmiller May 10, 2023 •

edited

Loading

jerelmiller May 10, 2023 •

edited

Loading

jerelmiller May 10, 2023

jerelmiller commented May 10, 2023 •

edited

Loading

benjamn May 10, 2023

benjamn May 10, 2023

jerelmiller May 10, 2023 •

edited

Loading

benjamn May 15, 2023

jerelmiller May 15, 2023

phryneas commented May 11, 2023

jerelmiller commented May 11, 2023

phryneas commented May 15, 2023

jerelmiller commented May 15, 2023

github-actions bot commented May 15, 2023 •

edited

Loading

Introduce the removeTypenameFromVariables link to remove __typename from variables #10853

Introduce the removeTypenameFromVariables link to remove __typename from variables #10853

Conversation

jerelmiller commented May 10, 2023 • edited Loading

Checklist:

changeset-bot bot commented May 10, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller May 10, 2023 • edited Loading

Choose a reason for hiding this comment

jerelmiller May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller commented May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented May 11, 2023

jerelmiller commented May 11, 2023

phryneas commented May 15, 2023

jerelmiller commented May 15, 2023

github-actions bot commented May 15, 2023 • edited Loading

size-limit report 📦

Introduce the `removeTypenameFromVariables` link to remove `__typename` from variables #10853

Introduce the `removeTypenameFromVariables` link to remove `__typename` from variables #10853

jerelmiller commented May 10, 2023 •

edited

Loading

changeset-bot bot commented May 10, 2023 •

edited

Loading

jerelmiller May 10, 2023 •

edited

Loading

jerelmiller May 10, 2023 •

edited

Loading

jerelmiller commented May 10, 2023 •

edited

Loading

jerelmiller May 10, 2023 •

edited

Loading

github-actions bot commented May 15, 2023 •

edited

Loading