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

Support InMemoryCache({ freezeResults: true }) to help enforce immutability. #4514

Merged
merged 3 commits into from
Feb 28, 2019

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Feb 28, 2019

Part of the plan I outlined in this comment: #4464 (comment)

If we could trust application code not to modify cache results, we wouldn't have to save deep snapshots of past results in order to implement isDifferentFromLastResult correctly (see #4069).

Aside: why doesn't the cache just return defensive copies of all results? #4031 (comment)

While you might agree that immutability is a worthwhile aspiration, it can be hard to maintain that discipline across your entire application over time, especially in a team of multiple developers.

This commit implements a new freezeResults option for the InMemoryCache constructor, which (when true) causes all cache results to be frozen in development, so you can more easily detect accidental mutations.

Note: mutating frozen objects only throws in strict mode, whereas it fails silently in non-strict code. ECMAScript module code automatically runs in strict mode, and most module transforms add "use strict" at the top of the generated code, so you're probably already using strict mode everywhere, though you might want to double-check.

The beauty of this implementation is that it does not need to repeatedly freeze entire results, because it can shallow-freeze the root of each subtree when that object is first created.

Thanks to result caching, those frozen objects can be shared between multiple different result trees without any additional freezing, and the entire result always ends up deeply frozen.

The freezing happens only in non-production environments, so there is no runtime cost to using { freezeResults: true } in production. Please keep this in mind when benchmarking cache performance!

…bility.

Part of the plan I outlined in this comment:
#4464 (comment)

If we could trust application code not to modify cache results, we
wouldn't have to save deep snapshots of past results in order to implement
isDifferentFromLastResult correctly (see #4069).

Aside: why doesn't the cache just return defensive copies of all results?
#4031 (comment)

While you might agree that immutability is a worthwhile aspiration, it can
be hard to maintain that discipline across your entire application over
time, especially in a team of multiple developers.

This commit implements a new freezeResults option for the InMemoryCache
constructor, which (when true) causes all cache results to be frozen in
development, so you can more easily detect accidental mutations.

Note: mutating frozen objects only throws in strict mode, whereas it fails
silently in non-strict code. ECMAScript module code automatically runs in
strict mode, and most module transforms add "use strict" at the top of the
generated code, so you're probably already using strict mode everywhere,
though you might want to double-check.

The beauty of this implementation is that it does not need to repeatedly
freeze entire results, because it can shallow-freeze the root of each
subtree when that object is first created.

Thanks to result caching, those frozen objects can be shared between
multiple different result trees without any additional freezing, and the
entire result always ends up deeply frozen.

The freezing happens only in non-production environments, so there is no
runtime cost to using { freezeResults: true } in production. Please keep
this in mind when benchmarking cache performance!
@benjamn benjamn self-assigned this Feb 28, 2019
@benjamn benjamn requested a review from hwillson as a code owner February 28, 2019 18:06
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.

Great solution @benjamn - thanks!

@benjamn benjamn merged commit 525d9ec into master Feb 28, 2019
@benjamn benjamn deleted the InMemoryCache-freezeResults-option branch February 28, 2019 20:55
benjamn added a commit that referenced this pull request Mar 6, 2019
Part of the plan I outlined in this comment:
#4464 (comment)

Passing { assumeImmutableResults: true } to the ApolloClient constructor
should probably always be accompanied by passing { freezeResults: true }
to the InMemoryCache constructor (see #4514), though of course the use of
InMemoryCache is optional, and other cache implementations may not support
that option.
benjamn pushed a commit that referenced this pull request May 20, 2019
The `readQuery` call may return an object that is shared with the current contents of cache, which can cause some weird behavior if you mutate the `readQuery` result.

See also:
#4514
#4543
@lifeiscontent
Copy link
Contributor

@benjamn can you share an example of what you should do versus what not to do when this option is enabled? I just want to understand the responsibilities necessary for this option in the different surface areas of apollo.

@benjamn
Copy link
Member Author

benjamn commented Aug 1, 2019

@lifeiscontent There's a quick example in the Apollo Client 2.6 blog post, in the Rewarding immutability section.

Don't do this:

const data = client.readQuery({
  query: CommentQuery,
});

// Destructive!
data.comments.push(newComment);

client.writeQuery({
  query: CommentQuery,
  data,
});

Do this instead:

const data = client.readQuery({
  query: CommentQuery
});

client.writeQuery({
  query: CommentQuery,
  data: {
    ...data,
    comments: [
      ...data.comments,
      newComment,
    ],
  },
});

Also, if your bundling tools are not already adding "use strict" to the top of your modules, you should find a way to do that, so that mutations of frozen objects will throw (in development), instead of failing silently.

@lifeiscontent
Copy link
Contributor

@benjamn wow, I didn't know about the post, the future of Apollo looks awesome. 🤩

benjamn added a commit that referenced this pull request Dec 2, 2019
The previousResult option was originally a way to ensure referential
identity of structurally equivalent cache results, before the result
caching system was introduced in #3394. It worked by returning
previousResult whenever it was deeply equal to the new result.

The result caching system works a bit differently, and in particular never
needs to do a deep comparison of results. However, there were still a few
(test) cases where previousResult seemed to have a positive effect, and
removing it seemed like a breaking change, so we kept it around.

In the meantime, the equality check has continued to waste CPU cycles, and
the behavior of previousResult has undermined other improvements, such as
freezing cache results (#4514). Even worse, previousResult effectively
disabled an optimization that allowed InMemoryCache#broadcastWatches to
skip unchanged queries (see comments I removed if curious). This commit
restores that optimization.

I realized eliminating previousResult might finally be possible while
working on PR #5617, which made the result caching system more precise by
depending on IDs+fields rather than just IDs. This additional precision
seems to have eliminated the few remaining cases where previousResult had
any meaningful benefit, as evidenced by the lack of any test changes in
this commit... even among the many direct tests of previousResult in
__tests__/diffAgainstStore.ts!

The removal of previousResult is definitely a breaking change (appropriate
for Apollo Client 3.0), because you can still contrive cases where some
never-before-seen previousResult object just happens to be deeply equal to
the new result. Also, it's fair to say that this removal will strongly
discourage disabling the result caching system (which is still possible
for diagnostic purposes), since we rely on result caching to get the
benefits that previousResult provided.
benjamn added a commit that referenced this pull request Dec 3, 2019
The previousResult option was originally a way to ensure referential
identity of structurally equivalent cache results, before the result
caching system was introduced in #3394. It worked by returning
previousResult whenever it was deeply equal to the new result.

The result caching system works a bit differently, and in particular never
needs to do a deep comparison of results. However, there were still a few
(test) cases where previousResult seemed to have a positive effect, and
removing it seemed like a breaking change, so we kept it around.

In the meantime, the equality check has continued to waste CPU cycles, and
the behavior of previousResult has undermined other improvements, such as
freezing cache results (#4514). Even worse, previousResult effectively
disabled an optimization that allowed InMemoryCache#broadcastWatches to
skip unchanged queries (see comments I removed if curious). This commit
restores that optimization.

I realized eliminating previousResult might finally be possible while
working on PR #5617, which made the result caching system more precise by
depending on IDs+fields rather than just IDs. This additional precision
seems to have eliminated the few remaining cases where previousResult had
any meaningful benefit, as evidenced by the lack of any test changes in
this commit... even among the many direct tests of previousResult in
__tests__/diffAgainstStore.ts!

The removal of previousResult is definitely a breaking change (appropriate
for Apollo Client 3.0), because you can still contrive cases where some
never-before-seen previousResult object just happens to be deeply equal to
the new result. Also, it's fair to say that this removal will strongly
discourage disabling the result caching system (which is still possible
for diagnostic purposes), since we rely on result caching to get the
benefits that previousResult provided.
benjamn added a commit that referenced this pull request Dec 3, 2019
The previousResult option was originally a way to ensure referential
identity of structurally equivalent cache results, before the result
caching system was introduced in #3394. It worked by returning
previousResult whenever it was deeply equal to the new result.

The result caching system works a bit differently, and in particular never
needs to do a deep comparison of results. However, there were still a few
(test) cases where previousResult seemed to have a positive effect, and
removing it seemed like a breaking change, so we kept it around.

In the meantime, the equality check has continued to waste CPU cycles, and
the behavior of previousResult has undermined other improvements, such as
freezing cache results (#4514). Even worse, previousResult effectively
disabled an optimization that allowed InMemoryCache#broadcastWatches to
skip unchanged queries (see comments I removed if curious). This commit
restores that optimization.

I realized eliminating previousResult might finally be possible while
working on PR #5617, which made the result caching system more precise by
depending on IDs+fields rather than just IDs. This additional precision
seems to have eliminated the few remaining cases where previousResult had
any meaningful benefit, as evidenced by the lack of any test changes in
this commit... even among the many direct tests of previousResult in
src/cache/inmemory/__tests__/diffAgainstStore.ts!

The removal of previousResult is definitely a breaking change (appropriate
for Apollo Client 3.0), because you can still contrive cases where some
never-before-seen previousResult object just happens to be deeply equal to
the new result. Also, it's fair to say that this removal will strongly
discourage disabling the result caching system (which is still possible
for diagnostic purposes), since we rely on result caching to get the
benefits that previousResult provided.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants