-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatically strip __typename
for input data in variables
#10724
Conversation
🦋 Changeset detectedLatest commit: 12d4dae The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This omitDeep
utility seems super useful! A few suggestions for perfecting the implementation.
src/utilities/common/omitDeep.ts
Outdated
if (isNonNullObject(value)) { | ||
const obj = Object.create(Object.getPrototypeOf(value)); | ||
known.set(value, obj); | ||
|
||
Object.keys(value).forEach((k) => { | ||
if (k !== key) { | ||
obj[k] = __omitDeep(value[k], key, known); | ||
} | ||
}); | ||
|
||
return obj; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we take advantage of situations where value
does not contain key
, and none of its children/descendents do, either?
I'm imagining __omitDeep
could sometimes return its input value
unmodified even if it's an object or an array, because there was nothing to modify, and then you could take advantage of those shortcuts by not always creating a new obj
object here (if all the child values come back ===
and value
does not contain key
).
Ideally, if you pass a large object tree to omitDeep
that doesn't have the key
anywhere within it, you'd get back the same (===
) object, not a deep copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a really great point! I'll add this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this behavior in 72c7d98. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed some additional behavior in c741fbf
…prevent reaching into another package-scoped bundle.
b6302e4
to
72c7d98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one extra safety consideration (see comment below).
This PR is ready to go. I'll rebase and merge once the type issues are fixed on the base |
…l/apollo-client into strip-typename-in-mutations
Closes apollographql/apollo-feature-requests#6
A highly requested feature of ours is the ask for the library to automatically strip
__typename
fields for arguments passed to a mutation. Because we automatically request__typename
as part of the GraphQL document, this can cause issues when trying to use that query data as an input argument to a mutation since the GraphQL spec states that input fields must not have a name that begins with__
(two underscores).Looking through that feature request, you'll notice many have attempted to solve this in creative ways. While a lot of the solutions are stripping them correctly (e.g. stripping
__typename
invariables
), there is risk that a naive implementation might either try and stop the__typename
from being requested at all, or by trying to post-process the query result in a custom link and removing the__typename
field. These solutions are ill-advised as the cache and type policies rely on__typename
to reliably work.Instead of asking our users to get creative with ways to strip
__typename
, this PR instead will automatically stripvariables
of any__typename
field (perhaps deeply nested) for any query, mutation, or subscription sent throughHttpLink
orBatchHttpLink
, orGraphQLWsLink
. This allows users to use existing query data as input to other GraphQL operations more naturally (most commonly mutations).As an example, say I have a query that requests a
todo
:Now lets say I have a mutation that allows me to update that todo in some way. An easy way to do this would be to reuse the
data
returned from my query with updated property values.This mutation will fail however since the
__typename
returned from the query is still present indata.todo
which is sent to the server.While we could ask users to strip
__typename
out, as demonstrated by the feature request, this is done in many different ways. I'd also argue that for more "beginner" users (users that are new to Apollo Client), its not apparent that__typename
is even there. If you look at the code samples above, nowhere do you see this field requested, so its not visibly apparent that you are going to run into issues.I find this small addition to be a huge quality of life improvement, especially since the default behavior of the client is to automatically add
__typename
to all outgoing GraphQL documents.Checklist: