Skip to content

Commit 59ff9c1

Browse files
captbaritonefacebook-github-bot
authored andcommitted
Mark resolves as clean again if we reread their fragment and find data unchanged
Reviewed By: monicatang Differential Revision: D56209294 fbshipit-source-id: 21d2884618a3358b1d7c35ffbb18480d4d9174e3
1 parent ec5f924 commit 59ff9c1

File tree

4 files changed

+102
-53
lines changed

4 files changed

+102
-53
lines changed

packages/relay-runtime/store/ResolverCache.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import type {
2626

2727
const recycleNodesInto = require('../util/recycleNodesInto');
2828
const {RELAY_LIVE_RESOLVER} = require('../util/RelayConcreteNode');
29+
const RelayFeatureFlags = require('../util/RelayFeatureFlags');
2930
const shallowFreeze = require('../util/shallowFreeze');
3031
const {generateClientID} = require('./ClientID');
3132
const RelayModernRecord = require('./RelayModernRecord');
@@ -325,6 +326,22 @@ class RecordResolverCache implements ResolverCache {
325326
if (recycled !== originalInputs) {
326327
return true;
327328
}
329+
330+
if (RelayFeatureFlags.MARK_RESOLVER_VALUES_AS_CLEAN_AFTER_FRAGMENT_REREAD) {
331+
// This record does not need to be recomputed, we can reuse the cached value.
332+
// For subsequent reads we can mark this record as "clean" so that they will
333+
// not need to re-read the fragment.
334+
const nextRecord = RelayModernRecord.clone(record);
335+
RelayModernRecord.setValue(
336+
nextRecord,
337+
RELAY_RESOLVER_INVALIDATION_KEY,
338+
false,
339+
);
340+
341+
const recordSource = this._getRecordSource();
342+
recordSource.set(RelayModernRecord.getDataID(record), nextRecord);
343+
}
344+
328345
return false;
329346
}
330347

packages/relay-runtime/store/__tests__/RelayReader-Resolver-test.js

Lines changed: 64 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -522,65 +522,76 @@ describe.each([
522522
expect(resolverInternals._relayResolverTestCallCount).toBe(1);
523523
});
524524

525-
it('marks the resolver cache as clean if the upstream has not changed', () => {
526-
const source = RelayRecordSource.create({
527-
'client:root': {
528-
__id: 'client:root',
529-
__typename: '__Root',
530-
me: {__ref: '1'},
531-
},
532-
'1': {
533-
__id: '1',
534-
id: '1',
535-
__typename: 'User',
536-
name: 'Alice',
537-
},
538-
});
525+
it.each([true, false])(
526+
'marks the resolver cache as clean if the upstream has not changed with RelayFeatureFlags.MARK_RESOLVER_VALUES_AS_CLEAN_AFTER_FRAGMENT_REREAD=%s',
527+
markClean => {
528+
RelayFeatureFlags.MARK_RESOLVER_VALUES_AS_CLEAN_AFTER_FRAGMENT_REREAD =
529+
markClean;
530+
const source = RelayRecordSource.create({
531+
'client:root': {
532+
__id: 'client:root',
533+
__typename: '__Root',
534+
me: {__ref: '1'},
535+
},
536+
'1': {
537+
__id: '1',
538+
id: '1',
539+
__typename: 'User',
540+
name: 'Alice',
541+
},
542+
});
539543

540-
const store = new RelayStore(source, {gcReleaseBufferSize: 0});
541-
const environment = new RelayModernEnvironment({
542-
network: RelayNetwork.create(jest.fn()),
543-
store,
544-
});
544+
const store = new RelayStore(source, {gcReleaseBufferSize: 0});
545+
const environment = new RelayModernEnvironment({
546+
network: RelayNetwork.create(jest.fn()),
547+
store,
548+
});
545549

546-
const FooQuery = graphql`
547-
query RelayReaderResolverTestMarkCleanQuery {
548-
me {
549-
constant_dependent
550+
const FooQuery = graphql`
551+
query RelayReaderResolverTestMarkCleanQuery {
552+
me {
553+
constant_dependent
554+
}
550555
}
551-
}
552-
`;
556+
`;
553557

554-
const cb = jest.fn<[Snapshot], void>();
555-
const operation = createOperationDescriptor(FooQuery, {});
556-
const snapshot = store.lookup(operation.fragment);
557-
const subscription = store.subscribe(snapshot, cb);
558-
// $FlowFixMe[unclear-type] - lookup() doesn't have the nice types of reading a fragment through the actual APIs:
559-
const {me}: any = snapshot.data;
560-
expect(me.constant_dependent).toEqual(1);
561-
environment.commitUpdate(theStore => {
562-
const alice = nullthrows(theStore.get('1'));
563-
alice.setValue('Alicia', 'name');
564-
});
565-
subscription.dispose();
558+
const cb = jest.fn<[Snapshot], void>();
559+
const operation = createOperationDescriptor(FooQuery, {});
560+
const snapshot = store.lookup(operation.fragment);
561+
const subscription = store.subscribe(snapshot, cb);
562+
// $FlowFixMe[unclear-type] - lookup() doesn't have the nice types of reading a fragment through the actual APIs:
563+
const {me}: any = snapshot.data;
564+
expect(me.constant_dependent).toEqual(1);
565+
environment.commitUpdate(theStore => {
566+
const alice = nullthrows(theStore.get('1'));
567+
alice.setValue('Alicia', 'name');
568+
});
569+
subscription.dispose();
570+
571+
// Rereading the resolver's fragment, only to find that no fields that we read have changed,
572+
// should clear the RELAY_RESOLVER_INVALIDATION_KEY.
573+
const resolverCacheRecord = environment
574+
.getStore()
575+
.getSource()
576+
.get('client:1:constant_dependent');
577+
invariant(
578+
resolverCacheRecord != null,
579+
'Expected a resolver cache record',
580+
);
566581

567-
// Rereading the resolver's fragment, only to find that no fields that we read have changed,
568-
// should clear the RELAY_RESOLVER_INVALIDATION_KEY.
569-
const resolverCacheRecord = environment
570-
.getStore()
571-
.getSource()
572-
.get('client:1:constant_dependent');
573-
invariant(resolverCacheRecord != null, 'Expected a resolver cache record');
574-
575-
const isMaybeInvalid = RelayModernRecord.getValue(
576-
resolverCacheRecord,
577-
RELAY_RESOLVER_INVALIDATION_KEY,
578-
);
582+
const isMaybeInvalid = RelayModernRecord.getValue(
583+
resolverCacheRecord,
584+
RELAY_RESOLVER_INVALIDATION_KEY,
585+
);
579586

580-
// TODO(T185969900) This should actually be false since the reread of the fragment
581-
// should have reset the RELAY_RESOLVER_INVALIDATION_KEY flag.
582-
expect(isMaybeInvalid).toBe(true);
583-
});
587+
if (markClean) {
588+
expect(isMaybeInvalid).toBe(false);
589+
} else {
590+
// Without the feature flag enabled, T185969900 still reproduces.
591+
expect(isMaybeInvalid).toBe(true);
592+
}
593+
},
594+
);
584595

585596
it('handles optimistic updates (applied after subscribing)', () => {
586597
const source = RelayRecordSource.create({

packages/relay-runtime/store/experimental-live-resolvers/LiveResolverCache.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import type {LiveState} from 'relay-runtime';
3535

3636
const recycleNodesInto = require('../../util/recycleNodesInto');
3737
const {RELAY_LIVE_RESOLVER} = require('../../util/RelayConcreteNode');
38+
const RelayFeatureFlags = require('../../util/RelayFeatureFlags');
3839
const shallowFreeze = require('../../util/shallowFreeze');
3940
const {generateClientID, generateClientObjectClientID} = require('../ClientID');
4041
const RelayModernRecord = require('../RelayModernRecord');
@@ -699,6 +700,22 @@ class LiveResolverCache implements ResolverCache {
699700
if (recycled !== originalInputs) {
700701
return true;
701702
}
703+
704+
if (RelayFeatureFlags.MARK_RESOLVER_VALUES_AS_CLEAN_AFTER_FRAGMENT_REREAD) {
705+
// This record does not need to be recomputed, we can reuse the cached value.
706+
// For subsequent reads we can mark this record as "clean" so that they will
707+
// not need to re-read the fragment.
708+
const nextRecord = RelayModernRecord.clone(record);
709+
RelayModernRecord.setValue(
710+
nextRecord,
711+
RELAY_RESOLVER_INVALIDATION_KEY,
712+
false,
713+
);
714+
715+
const recordSource = this._getRecordSource();
716+
recordSource.set(RelayModernRecord.getDataID(record), nextRecord);
717+
}
718+
702719
return false;
703720
}
704721

packages/relay-runtime/util/RelayFeatureFlags.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ export type FeatureFlags = {
4848
ENABLE_FIELD_ERROR_HANDLING_CATCH_DIRECTIVE: boolean,
4949

5050
PROCESS_OPTIMISTIC_UPDATE_BEFORE_SUBSCRIPTION: boolean,
51+
52+
// Temporary flag to enable a gradual rollout of the fix for T185969900
53+
MARK_RESOLVER_VALUES_AS_CLEAN_AFTER_FRAGMENT_REREAD: boolean,
5154
};
5255

5356
const RelayFeatureFlags: FeatureFlags = {
@@ -71,6 +74,7 @@ const RelayFeatureFlags: FeatureFlags = {
7174
ENABLE_FIELD_ERROR_HANDLING_THROW_BY_DEFAULT: false,
7275
ENABLE_FIELD_ERROR_HANDLING_CATCH_DIRECTIVE: false,
7376
PROCESS_OPTIMISTIC_UPDATE_BEFORE_SUBSCRIPTION: false,
77+
MARK_RESOLVER_VALUES_AS_CLEAN_AFTER_FRAGMENT_REREAD: false,
7478
};
7579

7680
module.exports = RelayFeatureFlags;

0 commit comments

Comments
 (0)