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

Warn when clobbering non-normalized data in the cache. #5833

Closed

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jan 23, 2020

One consequence of #5603 is that replacing non-normalized data in the cache can result in loss of useful data, which is preferable to mistakenly merging unrelated objects.

In almost every case, the right solution is to make sure the data can be normalized, or (if that isn't possible) to define a custom merge function for the replaced field, within the parent TypePolicy.

It turns out we can give a very detailed warning in all such situations, so that's what this commit does. Looking at the output for our test suite, every warning is legitimate and worth fixing. I will resolve the warnings and test failures that our test suite generates in subsequent commits.

@benjamn
Copy link
Member Author

benjamn commented Jan 23, 2020

Here's an example of one of the warnings:

Cache data may be lost when replacing the d field of a Query object.

To address this problem (which is not a bug in Apollo Client), either ensure that objects of type D have IDs, or define a custom merge function for the Query.d field, so the InMemoryCache can safely merge these objects:

  existing: {"__typename":"D","e":4}
  incoming: {"__typename":"D","h":{"__typename":"H","i":7}}

For more information about these options, please refer to the documentation:

  * Ensuring entity objects have IDs: https://deploy-preview-5677--apollo-client-docs.netlify.com/docs/react/v3.0-beta/caching/cache-configuration/#generating-unique-identifiers

  * Defining custom merge functions: https://deploy-preview-5677--apollo-client-docs.netlify.com/docs/react/v3.0-beta/caching/cache-field-behavior/#merging-non-normalized-objects

@benjamn benjamn force-pushed the warn-about-replacing-non-normalized-data-in-cache branch from 86a2bba to 2fe7b28 Compare January 24, 2020 15:58
@benjamn benjamn removed the request for review from jbaxleyiii January 24, 2020 20:57
One consequence of #5603 is that replacing non-normalized data in the
cache can result in loss of useful data. In almost every case, the right
solution is to make sure the data can be normalized, or (if that isn't
possible) to define a custom merge function for the replaced field, within
the parent TypePolicy.

It turns out we can give a very detailed warning in all such situations,
so that's what this commit does. Looking at the output for our test suite,
every warning is legitimate and worth fixing. I will resolve the warnings
and test failures that our test suite generates in subsequent commits.
Replacing a Reference in the cache is always safe, even with
non-normalized data, since the Reference refers to data safely stored
elsewhere in the cache.
It's much easier for the Policies code to check whether there's a merge
function defined for a given parent __typename and field name.
@benjamn benjamn force-pushed the warn-about-replacing-non-normalized-data-in-cache branch from cbeeafc to bc2712d Compare January 24, 2020 22:37
Warning about data loss is rightfully a concern of the StoreWriter, not
Policies. Also, StoreWriter has easy access to policies.hasMergeFunction,
which was the main selling point for putting warnAboutDataLoss in the
policies.ts module.
benjamn added a commit that referenced this pull request Jan 25, 2020
Follow-up to #5680.

Assuming keyArgs:false just because a merge function (or a read function)
was defined means that the following pattern can actually change cache
behavior in surprising ways:

  const cache = new InMemoryCache({
    typePolicies: {
      Person: {
        fields: {
          friends: {
            // This is supposed to be identical to what the cache does by
            // default, except without warnings about data loss, but it
            // subtly alters the default argument-based field identity
            // behavior when arguments are provided to the friends field.
            merge(_, incoming) {
              return incoming;
            },
          },
        },
      },
    },
  });

Although this merge function mirrors how the cache behaves without a merge
function, it has the benefit of making that behavior explicit, thereby
silencing the warnings introduced by #5833. In order to recommend this
strategy for silencing warnings when a last-write-wins merge strategy is
desired, it seems unacceptable for this pattern to have any unexpected
consequences beyond silencing the warnings.

Thinking about this default keyArgs:false behavior more, disabling field
identity with keyArgs:false doesn't really make sense if you have only a
merge function, or only a read function, since any fancy argument handling
done by one of those functions cannot be balanced in the other direction.
benjamn added a commit that referenced this pull request Jan 25, 2020
Follow-up to #5680.

Assuming keyArgs:false just because a merge function (or a read function)
was defined means that the following pattern can actually change cache
behavior in surprising ways:

  const cache = new InMemoryCache({
    typePolicies: {
      Person: {
        fields: {
          friends: {
            // This is supposed to be identical to what the cache does by
            // default, except without warnings about data loss, but it
            // subtly alters the default argument-based field identity
            // behavior when arguments are provided to the friends field.
            merge(_, incoming) {
              return incoming;
            },
          },
        },
      },
    },
  });

Although this merge function mirrors how the cache behaves without a merge
function, it has the benefit of making that behavior explicit, thereby
silencing the warnings introduced by #5833. In order to recommend this
strategy for silencing warnings when a last-write-wins merge strategy is
desired, it seems unacceptable for this pattern to have any unexpected
consequences beyond silencing the warnings.

Thinking about this default keyArgs:false behavior more, disabling field
identity with keyArgs:false doesn't really make sense if you have only a
merge function, or only a read function, since any fancy argument handling
done by one of those functions cannot be balanced in the other direction.
benjamn added a commit that referenced this pull request Jan 28, 2020
Follow-up to #5680.

Assuming keyArgs:false just because a merge function (or a read function)
was defined means that the following pattern can actually change cache
behavior in surprising ways:

  const cache = new InMemoryCache({
    typePolicies: {
      Person: {
        fields: {
          friends: {
            // This is supposed to be identical to what the cache does by
            // default, except without warnings about data loss, but it
            // subtly alters the default argument-based field identity
            // behavior when arguments are provided to the friends field.
            merge(_, incoming) {
              return incoming;
            },
          },
        },
      },
    },
  });

Although this merge function mirrors how the cache behaves without a merge
function, it has the benefit of making that behavior explicit, thereby
silencing the warnings introduced by #5833. In order to recommend this
strategy for silencing warnings when a last-write-wins merge strategy is
desired, it seems unacceptable for this pattern to have any unexpected
consequences beyond silencing the warnings.

Thinking about this default keyArgs:false behavior more, disabling field
identity with keyArgs:false doesn't really make sense if you have only a
merge function, or only a read function, since any fancy argument handling
done by one of those functions cannot be balanced in the other direction.
@codecov
Copy link

codecov bot commented Feb 9, 2020

Codecov Report

Merging #5833 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5833   +/-   ##
=======================================
  Coverage   94.53%   94.53%           
=======================================
  Files          91       91           
  Lines        3679     3698   +19     
  Branches      897      869   -28     
=======================================
+ Hits         3478     3496   +18     
  Misses        179      179           
- Partials       22       23    +1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 099e513...b59d244. Read the comment docs.


For more information about these options, please refer to the documentation:

* Ensuring entity objects have IDs: https://go.apollo.dev/c/generating-unique-identifiers
Copy link
Member

Choose a reason for hiding this comment

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

@benjamn These new links are temporarily redirecting to the longer deploy
preview links (we can update them to point to the proper prod docs location when this PR is merged). I took a guess at the slug names in these links; let me know if you want them changed/shortened further.

benjamn added a commit that referenced this pull request Jun 1, 2020
> This is a revival of PR #5833, which has accumulated too many merge
conflicts over the last few months to be worth rebasing.

One consequence of #5603 is that replacing non-normalized data in the
cache can result in loss of useful data, which is preferable to mistakenly
merging unrelated objects, but not ideal.

In almost every case, the right solution is to make sure the data can be
normalized, or (if that isn't possible) to define a custom `merge`
function for the field in question, within the parent type policy.

It turns out we can give a very detailed warning about such situations in
development, and that's what this commit does. For example:

  Cache data may be lost when replacing the d field of a Query object.

  To address this problem (which is not a bug in Apollo Client),
  either ensure all objects of type D have IDs, or define a custom
  merge function for the Query.d field, so InMemoryCache can safely
  merge these objects:

    existing: {"__typename":"D","e":4}
    incoming: {"__typename":"D","h":{"__typename":"H","i":7}}

  For more information about these options, please refer to the
  documentation:

    * Ensuring entity objects have IDs: https://go.apollo.dev/c/generating-unique-identifiers
    * Defining custom merge functions: https://go.apollo.dev/c/merging-non-normalized-objects

Looking at the output for our test suite, every warning seems legitimate
and worth fixing. I will resolve the warnings in subsequent commits.
@benjamn
Copy link
Member Author

benjamn commented Jun 1, 2020

Superseded by #6372.

@benjamn benjamn closed this Jun 1, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 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.

2 participants