Add the ability to allow @client fields to be sent to the link chain

[jerelmiller]

Closes #10303

By default, Apollo core will remove fields with an @client directive before it makes it to the link chain. We've now got use cases that require the use of @client in the link chain. This PR adds an option to our client to allow @client fields to make it to the link chain. This is part of our larger effort to move local resolvers back into the link chain.

Because @client fields now have the possibility of making it to the link chain, the HttpLink and BatchHttpLink links have been updated to remove @client fields themselves. Once #10060 is done, we should be able to remove this behavior as the work to remove @client fields will likely end up in the new Apollo link instead.

Design

The API introduced in this PR is as follows:

new ApolloClient({
  defaultOptions: {
    transformQuery: {
      removeClientFields: false
    }
  }
});

Why is transformQuery an object type?

I for-see the possibility of wanting more query transformation options in our client. Rather than trying to invent new names for top-level options (one of the 2 hardest things in programming), keeping them in a cohesive namespace felt like it would scale better. Imagine for example if we wanted to offer the possibility of removing whitespace from queries before they are sent to the server:

transformQuery: {
  stripWhitespace: true
}

This reads nicely and keeps the name of the new option concise.

While we don't have any plans for additional transformation options at this time, I wanted something that could scale as we have/need more use cases.

Why defaultOptions?

Thinking long-term, I could see the possibility of wanting to opt in/out of this behavior for individual queries. We allow other options (e.g. fetchPolicy) to have default global values which can also be overridden on a per-query basis. To keep with the spirit of this notion, I aimed to introduce this under defaultOptions. Because these options are applied across all query types (query and mutation), I opted for a new "top-level" field under defaultOptions to convey that these are applied to all queries.

Why not allow individual queries to opt in/out of this behavior now?

While I highly considered adding this behavior, something that gave me pause is how we handle our addTypename feature. This option also performs query transformation, but currently this is only available as a global option. There is no way to opt in/out of this behavior for an individual query. I decided to keep things simple as this has worked well for us thus far. We can always add this functionality later.

Checklist:

• 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

[changeset-bot]

🦋 Changeset detected

Latest commit: 584bd40

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

[alessbell]

Very excited for what this will unlock for online/offline use cases like the one detailed in the RFC linked in #10303!

I'm in favor of your design: defaultOptions seems like the right place for this setting and I like transformQuery as a place to potentially include other options in the future as needed, as you've said. My only small nit would be in the naming of removeClientFields: my preference is to avoid double negatives (ie I find it less confusing when passing false disables the thing). What do you think about:

new ApolloClient({
  defaultOptions: {
    transformQuery: {
      clientFields: false
    }
  }
});

I find this intuitive especially since the "transform" in transformQuery implies "remove" in the false case (and the true case will rarely be typed out as it's the default, but still makes sense to me as conveying they should be present). My only other question is whether transformQuery should mention Links or the link chain - let me know what you think there.

[jerelmiller]

my preference is to avoid double negatives

@alessbell you make a really good point here. One of the greatest pieces of advice I got early on in my career was to avoid negatives as its harder for the brain to think in negatives than positives. I can definitely see merit in going this route.

---

To give some idea on why I chose this particular name, I tend to think of the option name as the thing that happens when your query goes from a -> b, where a is the original untouched query and b is the transformed query. In this vein, the option name describes what we are doing to the query as its getting transformed. For example, we add __typename to the query as part of the transformation, so if we were to move that into the transformQuery options (not a real suggestion, just an example), I'd name it addTypename to describe the transformation from a, which has no __typename fields, to b which does. In this case, the transform query options would be named like this:

transformQuery: {
  addTypename: true,
  removeClientFields: false,
  stripWhitespace: true
}

---

On the flip side, if we like the positive connotation, I'd make 1 small tweak to your suggestion and call it keepClientFields to keep a verb in the name, since a transform is actionable. Using this naming convention, I could see the option names being something like this:

transformQuery: {
  keepClientFields: true,
  addTypename: true,
  maintainWhitespace: true
}

@alessbell what are your thoughts? Which naming convention do you prefer?

FWIW I like the a -> b only because thats how my brain thinks, but I'm also biased 😅

[alessbell]

I tend to think of the option name as the thing that happens when your query goes from a -> b

@jerelmiller that's a great point: the verb is important because all future options might not fall into the "always allow (passthrough) or never allow (remove)" case (whitespace, @client). There are others like the always add case (__typename) where false would not have a clear meaning.

I see the benefits of describing the transform action itself—names like maintainWhitespace are also awkward IMO—so this seems like a fair tradeoff. I cast my vote for the original convention 🗳😊 Thanks!

[alessbell]

LGTM! 🚢

[nyteshade]

@jerelmiller heya, so I am finally back from the holidays and wanted to give this a spin today. It looks like something got lost in translation.

The goal was never to have to manually go back and forth with the removeClientFields option but allow client fields to be exposed in the link, optionally.

Here is where this breaks down

We need to be able to work on the client fields if we have a need, but if we leave the operation with client directives in the query and/or mutation they still need to be processed as normal by the client. This seems to work for queries when removeClientFields: false is specified but fails on mutations. Happy to work with you all on this as needed.

[jerelmiller]

Hey @nyteshade! Welcome back!

The goal was never to have to manually go back and forth with the removeClientFields option but allow client fields to be exposed in the link, optionally.

Yep totally understand. I'm mainly thinking about this from a "how might this be used for a broad audience" kind of way, hence the discussion about being able to toggle this transform on or off per query. Turns out that touched quite a few code paths, so I've left it as a global option.

but fails on mutations

Ah this sounds like a bug to me. That wasn't intentional, just an oversight as I had though both queries and mutations go through the same code path. Definitely something we should fix! Glad to hear queries are working as expected at least!

Feel free to submit a PR if you'd like look at this, otherwise I'll try and get a fix in for this sometime in the coming week. Thanks for trying it out!