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

Implement ObservableQuery#isDifferentFromLastResult. #4069

Merged
merged 2 commits into from
Oct 30, 2018

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Oct 26, 2018

This commit is a more conservative version of e66027c.

We still need to make a deep clone of observableQuery.lastResult in order to determine if future results are different (see #3992), but we should be able to restrict the use of that snapshot to a single method, rather than replacing observableQuery.lastResult with the snapshot.

Should help with #4054 (cc @jsslai).

  • Diagnose and fix the failing tests.

This commit is a more conservative version of
e66027c

We still need to make a deep clone of observableQuery.lastResult in order
to determine if future results are different (see #3992), but we can
restrict the use of that snapshot to a single method, rather than
replacing observableQuery.lastResult with the snapshot.

Should help with #4054.
@benjamn benjamn force-pushed the issue-4054-ObservableQuery-isDifferentFromLastResult branch from 9db4980 to 510fc3c Compare October 30, 2018 14:56
@benjamn benjamn merged commit 905001e into master Oct 30, 2018
@benjamn benjamn deleted the issue-4054-ObservableQuery-isDifferentFromLastResult branch October 30, 2018 15:35
benjamn added a commit that referenced this pull request Feb 28, 2019
…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!
@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.

1 participant