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

Remove freezeResults option, assuming always true. #5153

Merged
merged 3 commits into from
Aug 21, 2019

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Aug 9, 2019

This option was first added in Apollo Client 2.6, as a way to help enforce immutability of cache results: #4514

As promised in the Apollo Client 2.6 blog post, all cache results will be frozen/immutable in the next major version (3.0):

In Apollo Client 3.0, we intend to make both freezeResults and assumeImmutableResults true by default, in addition to using the TypeScript Readonly<T> type to discourage destructive modifications. In other words, if you embrace immutability now, you won’t have to do anything when it becomes mandatory.

I have not yet removed the assumeImmutableResults option from the ApolloClient constructor, since cache implementations other than apollo-cache-inmemory might not return immutable results.

@benjamn benjamn added this to the Release 3.0 milestone Aug 9, 2019
@benjamn benjamn self-assigned this Aug 9, 2019
@benjamn benjamn requested a review from hwillson August 9, 2019 16:12
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.

Looks great @benjamn - exciting! 🎉

@benjamn benjamn mentioned this pull request Aug 9, 2019
31 tasks
@jbaxleyiii
Copy link
Contributor

I'm really glad we are doing this, but I do wonder if we can prune more of the freezing behavior to be part of the dev only build? There are a couple call sites to maybeDeepFreeze that get turned into a noop in production but wouldn't it be better to just prune that all together in prod?

@benjamn
Copy link
Member Author

benjamn commented Aug 21, 2019

@jbaxleyiii There's one spot that I see in apollo-cache-inmemory/src/readFromStore.ts:

if (process.env.NODE_ENV !== 'production') {
  assertSelectionSetForIdValue(contextValue.store, field, readStoreResult.result);
  maybeDeepFreeze(readStoreResult);
}

As you can see, it's wrapped with a process.env.NODE_ENV check, so the call to maybeDeepFreeze will get stripped in production if folks are properly dead-code-eliminating those blocks, as React requires them to do.

However, that guard does not remove the import { maybeDeepFreeze } from 'apollo-utilities', which is a general problem with top-level import declarations. The good news is that apollo-cache-inmemory/lib/bundle.cjs.min.js does not end up containing maybeDeepFreeze, so it is possible to remove it completely from that bundle, though some version of it might still end up remaining in the apollo-utilities bundle, depending on how folks set up their production builds.

I think this situation will improve once we move the relevant apollo-utilities code into the new @apollo/client package, so Rollup has more insight into exactly which functions are used. In the meantime, I think I'm comfortable with leaving this one call to maybeDeepFreeze in place, since it's used to freeze scalar values, which might be objects or arrays.

@benjamn benjamn force-pushed the always-freeze-cache-results branch from f14d6b2 to af94014 Compare August 21, 2019 19:29
// In development, we need to clone scalar values so that they can be
// safely frozen with maybeDeepFreeze in readFromStore.ts. In production,
// it's cheaper to store the scalar values directly in the cache.
return process.env.NODE_ENV === 'production' ? value : cloneDeep(value);
Copy link
Member Author

Choose a reason for hiding this comment

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

@jbaxleyiii Thanks to your comment I realized this clone is necessary to avoid accidentally freezing user-provided scalar values in development, as I'm now testing in af94014.

@benjamn benjamn merged commit ad1fedb into release-3.0 Aug 21, 2019
@benjamn
Copy link
Member Author

benjamn commented Aug 21, 2019

Merging this preemptively so that @hwillson can begin work on restructuring the repo starting from the release-3.0 branch.

@benjamn benjamn deleted the always-freeze-cache-results branch August 21, 2019 19:34
schettn added a commit to snek-at/front that referenced this pull request Sep 9, 2020
Due to the immutable of talkList the list had to be
copied.
Ref: apollographql/apollo-client#5153
benjamn added a commit that referenced this pull request Feb 16, 2023
This commit allows the default value of the `assumeImmutableResults`
option to be determined by the cache (any implementation of
`ApolloCache`), which allows our implementation of `InMemoryCache` to
express the safety of assuming `assumeImmutableResults` is `true`,
unlocking significant performance savings (fewer defensive deep copies
of query results), even if `assumeImmutableResults` is not configured.

Related past PRs:
- #4543
- #5153
- #9680
- [Apollo Client 2.6 blog post](https://www.apollographql.com/blog/announcement/frontend/whats-new-in-apollo-client-2-6/#rewarding-immutability)
@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