Skip to content
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

Stop returning unwanted __typename fields in cache results. #5311

Closed
wants to merge 5 commits into from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Sep 12, 2019

This change was extracted from #5154, per @jbaxleyiii's approval.

If you read from the cache using a query that does not have explicit __typename fields, the cache should not return data containing the __typename field. Unfortunately, we have a bunch of tests that currently enforce the accidental presence of __typename, so this relatively simple change required updating a lot of test code.

This change was adapted from #5154, per @jbaxleyiii's approval:
#5154 (comment)

If you read from the cache using a query that does not have explicit
__typename fields, the cache should not return data containing the
__typename field. Unfortunately, we have a bunch of tests that currently
enforce the accidental presence of __typename, so this relatively simple
change required updating a lot of test code.
@@ -135,7 +135,7 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {

return this.storeReader.readQueryFromStore({
store: options.optimistic ? this.optimisticData : this.data,
query: this.transformDocument(options.query),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still call transformDocument when writing to the cache, so any __typename fields in the result object will be stored in the cache, but when we're reading we need to use the original query as provided by the application.

src/util/testUtils.ts Outdated Show resolved Hide resolved
@benjamn benjamn mentioned this pull request Sep 12, 2019
31 tasks
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it @benjamn - thanks!

Adding __typename fields to the query used to be necessary for writing
results to the cache, but now __typename fields in result objects are
automatically written to the cache, so no transformation is necessary.

ApolloClient still calls cache.transformDocument to get a query with
__typename fields to send to the server, and the cache still relies on
results from the server coming back with __typename fields, but the cache
no longer needs the query to be transformed to include __typename.
this.storeWriter.writeQueryToStore({
store: this.data,
query: this.transformDocument(write.query),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, we're no longer calling transformDocument or addTypenameToDocument anywhere in the InMemoryCache implementation!

A very common pattern (e.g. in mutation update functions) is to call
cache.readFragment, transform the result, then write the new result back
using cache.writeFragment.

With the recent changes to avoid returning unwanted __typename fields in
cache results, this pattern would have broken because fragments read from
the cache without adequate __typename information would be written back to
the cache without adequate __typename information, even if the cache could
have returned that __typename information.

We still don't want __typename showing up in result objects if the query
didn't ask for it, but we can include it in a consistent, hidden way by
adding __typename as a non-enumerable property of relevant result objects.
benjamn added a commit that referenced this pull request Sep 13, 2019
Adding __typename fields to the query by calling addTypenameToDocument
used to be necessary for writing results to the cache, but now __typename
fields in result objects are automatically written to the cache, so no
transformation is necessary.

ApolloClient still calls cache.transformDocument to get a query with
__typename fields to send to the server, and the cache still relies on
results from the server coming back with __typename fields, but the cache
no longer needs the query to be transformed to include __typename.

Compared to PR #5311, this commit should not be a breaking change.
@benjamn
Copy link
Member Author

benjamn commented Sep 13, 2019

Closing in favor of #5320, which guarantees __typename will be included in cache results as long as addTypename is true (which the the default behavior for InMemoryCache). That's roughly the opposite of what this PR tried to do, but it makes more sense, all things considered.

After further discussion with @jbaxleyiii, we've come to the conclusion that hiding __typename fields would have needlessly disrupted readFragment => transform => writeFragment workflows, because writeFragment needs __typename information to work properly, so readFragment must return that information.

The final hack that I implemented in 7d1df90 (including __typename fields but making them non-enumerable) was not sufficient, because object spread syntax only picks up own enumerable properties, so patterns like this would still fail:

const data = cache.readFragment({ id, fragment });
cache.writeFragment({
  data: {
    ...data, // fails to copy __typename
    todos: [mResult.data.createTodo, ...data.todos],
  },
  id,
  fragment,
});

@benjamn benjamn closed this Sep 13, 2019
benjamn added a commit that referenced this pull request Sep 13, 2019
…#5320)

Adding __typename fields to the query by calling addTypenameToDocument
used to be necessary for writing results to the cache, but now __typename
fields in result objects are automatically written to the cache, so no
transformation is necessary.

ApolloClient still calls cache.transformDocument to get a query with
__typename fields to send to the server, and the cache still relies on
results from the server coming back with __typename fields, but the cache
no longer needs the query to be transformed to include __typename.

Compared to PR #5311, this commit should not be a breaking change.
StephenBarlow pushed a commit that referenced this pull request Oct 1, 2019
…#5320)

Adding __typename fields to the query by calling addTypenameToDocument
used to be necessary for writing results to the cache, but now __typename
fields in result objects are automatically written to the cache, so no
transformation is necessary.

ApolloClient still calls cache.transformDocument to get a query with
__typename fields to send to the server, and the cache still relies on
results from the server coming back with __typename fields, but the cache
no longer needs the query to be transformed to include __typename.

Compared to PR #5311, this commit should not be a breaking change.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants