From 2cf9ac982f964590fddab8206257147652e79cab Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 26 May 2020 15:15:11 -0700 Subject: [PATCH] Type refinement uses per-type metadata (previously per-record) Summary: Changes from storing information about whether a *record* conforms to a given interface/union, to instead storing information about whether a given *concrete type* conforms to a given interface/union. The main advantage of this new approach is that we will have fewer cases where we're missing type discriminators - if we've ever seen a type/interface refinement on one record, we can reuse that information for all other records of the same type. For example, imagine that a client executes the following query: ``` query UrlQuery($id: ID!) { node(id: $id) { ... on Entity { url } } } ``` For id=X, and the cache contains ``` root: Record { ... node(id:"X"): Ref("X") } X: Record { __id: "X", __typename: "Story", id: "X", url: "https://..." } ``` Previously, because the `__isEntity` key was missing on the *record*, this query would be considered to have missing data, even though the record has a `url` value. With the new change, if we've already learned that `Story` implements `Entity`, we know that this record does too, and can correctly report that the query can be fulfilled from cache and isn't missing data. == Tradeoffs The approach here for storing type/interface conformance metadata is to reuse records, creating a record per concrete type, and storing interface conformance as fields on those type records. The upside of this is that all our existing logic works: new type metadata is merged with existing metadata, unneeded type metadata can be GC'd, etc. The downside is that to be precise, Reader should record that it has "seen" the type schema records. So with a fragment like: ``` fragment StoryHeader on Story { id name ... on Entity { url } } ``` This means that *every* subscription to StoryHeader will subscribe to *all* changes to the `Story` type schema record, in addition to their specific Story records. Any change to the Story type schema record (say we learn that Story conforms to the Foo interface) will cause us to re-evaluate *all* StoryHeaders fragments, even though they aren't likely to change. Possible alternatives: * Instead of creating type schema records *per-type*, create them *per type/interface pair*. So instead of having a `User`-type schema record with fields per interface, we'd have a `User/Entity` type schema record with a single `implements: boolean` field on it. This would avoid having to reread fragments in most cases, since the `implements` value would never change in practice for a given type/interface pair. * Another approach is to just assume that type/interface conformance will never change over the course of the session, and *not* record a dependency on the type schema record. == What About Updates There's also a question of whether we should notify fragment subscriptions if isMissingData changes. Right now we *only* notify if the `data` field changes, but it's possible that data could be the same while isMissingData changes (for example if we didn't have a type discriminator and it gets loaded later). Overall not notifying of isMissingData changes seems fine in practice but not ideal (though note that there is precedent for not notifying of all changes to a Snapshot - we don't notify if data has no changes but seenRecords does change). == Approach Given those constraints I think the most reasonable option is what's implemented here: * Store type/interface conformance per-type (so there's a single record for e.g. `User` with fields for each interface we refine it to) * Don't publish fragment subscription updates that only affect isMissingData (achieved by having reader look at the type schema record, without recording a dependency on it). Reviewed By: kassens Differential Revision: D21663869 fbshipit-source-id: 3faa602f90171295a5d857df400e4e60273aac54 --- packages/relay-runtime/store/DataChecker.js | 19 +- packages/relay-runtime/store/RelayReader.js | 24 ++- .../store/RelayReferenceMarker.js | 17 +- .../store/RelayResponseNormalizer.js | 19 +- packages/relay-runtime/store/TypeID.js | 28 +++ .../store/__tests__/DataChecker-test.js | 8 +- ...ayModernEnvironment-TypeRefinement-test.js | 200 +++++++++++------- .../store/__tests__/RelayReader-test.js | 66 ++++++ 8 files changed, 295 insertions(+), 86 deletions(-) create mode 100644 packages/relay-runtime/store/TypeID.js diff --git a/packages/relay-runtime/store/DataChecker.js b/packages/relay-runtime/store/DataChecker.js index 30c1041ddd3f6..29ce5ddb8f41a 100644 --- a/packages/relay-runtime/store/DataChecker.js +++ b/packages/relay-runtime/store/DataChecker.js @@ -25,6 +25,7 @@ const invariant = require('invariant'); const {isClientID} = require('./ClientID'); const {EXISTENT, UNKNOWN} = require('./RelayRecordState'); +const {generateTypeID} = require('./TypeID'); import type { NormalizationField, @@ -314,8 +315,15 @@ class DataChecker { // - Unknown whether the type implements the interface: don't check the selections // and treat the data as missing; we do this because the Relay Compiler // guarantees that the type discriminator will always be fetched. - const implementsInterface = this._mutator.getValue( + const recordType = this._mutator.getType(dataID); + invariant( + recordType != null, + 'DataChecker: Expected record `%s` to have a known type', dataID, + ); + const typeID = generateTypeID(recordType); + const implementsInterface = this._mutator.getValue( + typeID, abstractKey, ); if (implementsInterface === true) { @@ -370,8 +378,15 @@ class DataChecker { case TYPE_DISCRIMINATOR: if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) { const {abstractKey} = selection; - const implementsInterface = this._mutator.getValue( + const recordType = this._mutator.getType(dataID); + invariant( + recordType != null, + 'DataChecker: Expected record `%s` to have a known type', dataID, + ); + const typeID = generateTypeID(recordType); + const implementsInterface = this._mutator.getValue( + typeID, abstractKey, ); if (implementsInterface == null) { diff --git a/packages/relay-runtime/store/RelayReader.js b/packages/relay-runtime/store/RelayReader.js index e9cc3ef086c9c..e49cfcc90f0dc 100644 --- a/packages/relay-runtime/store/RelayReader.js +++ b/packages/relay-runtime/store/RelayReader.js @@ -41,6 +41,7 @@ const { getStorageKey, getModuleComponentKey, } = require('./RelayStoreUtils'); +const {generateTypeID} = require('./TypeID'); import type { ReaderFragmentSpread, @@ -127,10 +128,13 @@ class RelayReader { record != null && RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT ) { - const implementsInterface = RelayModernRecord.getValue( - record, - abstractKey, - ); + const recordType = RelayModernRecord.getType(record); + const typeID = generateTypeID(recordType); + const typeRecord = this._recordSource.get(typeID); + const implementsInterface = + typeRecord != null + ? RelayModernRecord.getValue(typeRecord, abstractKey) + : null; if (implementsInterface === false) { // Type known to not implement the interface isDataExpectedToBePresent = false; @@ -218,10 +222,14 @@ class RelayReader { const parentIsMissingData = this._isMissingData; const parentIsWithinUnmatchedTypeRefinement = this ._isWithinUnmatchedTypeRefinement; - const implementsInterface = RelayModernRecord.getValue( - record, - abstractKey, - ); + + const typeName = RelayModernRecord.getType(record); + const typeID = generateTypeID(typeName); + const typeRecord = this._recordSource.get(typeID); + const implementsInterface = + typeRecord != null + ? RelayModernRecord.getValue(typeRecord, abstractKey) + : null; this._isWithinUnmatchedTypeRefinement = parentIsWithinUnmatchedTypeRefinement || implementsInterface === false; diff --git a/packages/relay-runtime/store/RelayReferenceMarker.js b/packages/relay-runtime/store/RelayReferenceMarker.js index b63a8e5345c2b..3bf8773b14139 100644 --- a/packages/relay-runtime/store/RelayReferenceMarker.js +++ b/packages/relay-runtime/store/RelayReferenceMarker.js @@ -13,12 +13,15 @@ 'use strict'; const RelayConcreteNode = require('../util/RelayConcreteNode'); +const RelayFeatureFlags = require('../util/RelayFeatureFlags'); const RelayModernRecord = require('./RelayModernRecord'); const RelayStoreUtils = require('./RelayStoreUtils'); const cloneRelayHandleSourceField = require('./cloneRelayHandleSourceField'); const invariant = require('invariant'); +const {generateTypeID} = require('./TypeID'); + import type { NormalizationLinkedField, NormalizationModuleImport, @@ -134,6 +137,11 @@ class RelayReferenceMarker { if (typeName != null && typeName === selection.type) { this._traverseSelections(selection.selections, record); } + } else if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) { + const typeName = RelayModernRecord.getType(record); + const typeID = generateTypeID(typeName); + this._references.add(typeID); + this._traverseSelections(selection.selections, record); } else { this._traverseSelections(selection.selections, record); } @@ -172,8 +180,15 @@ class RelayReferenceMarker { break; case SCALAR_FIELD: case SCALAR_HANDLE: - case TYPE_DISCRIMINATOR: break; + case TYPE_DISCRIMINATOR: { + if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) { + const typeName = RelayModernRecord.getType(record); + const typeID = generateTypeID(typeName); + this._references.add(typeID); + } + break; + } case MODULE_IMPORT: this._traverseModuleImport(selection, record); break; diff --git a/packages/relay-runtime/store/RelayResponseNormalizer.js b/packages/relay-runtime/store/RelayResponseNormalizer.js index 86ab2c850e0f0..0294c961bad5d 100644 --- a/packages/relay-runtime/store/RelayResponseNormalizer.js +++ b/packages/relay-runtime/store/RelayResponseNormalizer.js @@ -43,6 +43,7 @@ const { TYPENAME_KEY, ROOT_ID, } = require('./RelayStoreUtils'); +const {generateTypeID, TYPE_SCHEMA_TYPE} = require('./TypeID'); import type {PayloadData} from '../network/RelayNetworkTypes'; import type { @@ -198,8 +199,15 @@ class RelayResponseNormalizer { } } else if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) { const implementsInterface = data.hasOwnProperty(abstractKey); + const typeName = RelayModernRecord.getType(record); + const typeID = generateTypeID(typeName); + let typeRecord = this._recordSource.get(typeID); + if (typeRecord == null) { + typeRecord = RelayModernRecord.create(typeID, TYPE_SCHEMA_TYPE); + this._recordSource.set(typeID, typeRecord); + } RelayModernRecord.setValue( - record, + typeRecord, abstractKey, implementsInterface, ); @@ -217,8 +225,15 @@ class RelayResponseNormalizer { if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) { const {abstractKey} = selection; const implementsInterface = data.hasOwnProperty(abstractKey); + const typeName = RelayModernRecord.getType(record); + const typeID = generateTypeID(typeName); + let typeRecord = this._recordSource.get(typeID); + if (typeRecord == null) { + typeRecord = RelayModernRecord.create(typeID, TYPE_SCHEMA_TYPE); + this._recordSource.set(typeID, typeRecord); + } RelayModernRecord.setValue( - record, + typeRecord, abstractKey, implementsInterface, ); diff --git a/packages/relay-runtime/store/TypeID.js b/packages/relay-runtime/store/TypeID.js new file mode 100644 index 0000000000000..042b20bdc9219 --- /dev/null +++ b/packages/relay-runtime/store/TypeID.js @@ -0,0 +1,28 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow strict + * @format + */ + +// flowlint ambiguous-object-type:error + +'use strict'; + +import type {DataID} from '../util/RelayRuntimeTypes'; + +const PREFIX = 'client:__type:'; +const TYPE_SCHEMA_TYPE = '__TypeSchema'; + +function generateTypeID(typeName: string): DataID { + return PREFIX + typeName; +} + +function isTypeID(id: DataID): boolean { + return id.indexOf(PREFIX) === 0; +} + +module.exports = {generateTypeID, isTypeID, TYPE_SCHEMA_TYPE}; diff --git a/packages/relay-runtime/store/__tests__/DataChecker-test.js b/packages/relay-runtime/store/__tests__/DataChecker-test.js index ea66f8fa49bdc..1bbd52b8f9920 100644 --- a/packages/relay-runtime/store/__tests__/DataChecker-test.js +++ b/packages/relay-runtime/store/__tests__/DataChecker-test.js @@ -23,6 +23,7 @@ const getRelayHandleKey = require('../../util/getRelayHandleKey'); const {check} = require('../DataChecker'); const {createNormalizationSelector} = require('../RelayModernSelector'); const {ROOT_ID} = require('../RelayStoreUtils'); +const {generateTypeID, TYPE_SCHEMA_TYPE} = require('../TypeID'); const { createMockEnvironment, generateAndCompile, @@ -2204,6 +2205,7 @@ describe('check()', () => { } } `); + const typeID = generateTypeID('NonNodeNoID'); const data = { 'client:root': { __id: 'client:root', @@ -2213,10 +2215,14 @@ describe('check()', () => { '1': { __id: '1', __typename: 'NonNodeNoID', - __isNode: false, // no 'id' bc not a Node name: 'Not a Node!', }, + [typeID]: { + __id: typeID, + __typename: TYPE_SCHEMA_TYPE, + __isNode: false, + }, }; const source = RelayRecordSource.create(data); const target = RelayRecordSource.create(); diff --git a/packages/relay-runtime/store/__tests__/RelayModernEnvironment-TypeRefinement-test.js b/packages/relay-runtime/store/__tests__/RelayModernEnvironment-TypeRefinement-test.js index 107b68deb65ce..3b26365ec5df5 100644 --- a/packages/relay-runtime/store/__tests__/RelayModernEnvironment-TypeRefinement-test.js +++ b/packages/relay-runtime/store/__tests__/RelayModernEnvironment-TypeRefinement-test.js @@ -25,6 +25,7 @@ const { createOperationDescriptor, } = require('../RelayModernOperationDescriptor'); const {getSingularSelector} = require('../RelayModernSelector'); +const {generateTypeID} = require('../TypeID'); const {generateAndCompile} = require('relay-test-utils-internal'); describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', () => { @@ -124,9 +125,18 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT = false; }); + // Commit the given payload, immediately running GC to prune any data + // that wouldn't be retained by the query + // eslint-disable-next-line no-shadow + function commitPayload(operation, payload) { + environment.retain(operation); + environment.commitPayload(operation, payload); + (environment.getStore(): $FlowFixMe).__gc(); + } + it('concrete spread on matching concrete type reads data and counts missing user fields as missing', () => { // with missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -154,7 +164,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(operation).status).toBe('missing'); // add missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -183,7 +193,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('concrete spread on non-matching concrete type reads data but does not count missing user fields as missing', () => { // with missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -212,7 +222,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(operation).status).toBe('missing'); // fields missing from conforming interface (Actor) // add missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -242,7 +252,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('concrete inline fragment on matching concrete type reads data and counts missing user fields as missing', () => { // with missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -270,7 +280,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(operation).status).toBe('missing'); // add missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -297,7 +307,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(operation).status).toBe('available'); }); it('concrete inline fragment on non-matching concrete type does not read data or count data as missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -324,7 +334,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('abstract spread on implementing type reads data and counts missing user fields as missing', () => { // with missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -352,7 +362,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(operation).status).toBe('missing'); // add missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -380,7 +390,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); it('abstract spread on non-implementing type reads data but does not count missing user fields as missing', () => { // with missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', // __isActor: 'User', // no value: means that on server, User no longer implements Actor @@ -409,7 +419,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(operation).status).toBe('missing'); // fields missing on concrete type }); it('abstract spread missing only the discriminator reads data and counts data as missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, // deleted from store below @@ -420,9 +430,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }, }); environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isActor')).toBe(true); - record.setValue(undefined, '__isActor'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(true); + typeRecord.setValue(undefined, '__isActor'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -440,10 +450,20 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); expect(fragmentSnapshot.isMissingData).toBe(true); expect(environment.check(operation).status).toBe('missing'); + + // Subscriptions are not notified of discriminator-only changes + const callback = jest.fn(); + environment.subscribe(fragmentSnapshot, callback); + environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(undefined); + typeRecord.setValue(false, '__isActor'); + }); + expect(callback).toBeCalledTimes(0); }); it('abstract spread missing the discriminator and user fields: reads data and counts data as missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, // deleted from store below @@ -456,9 +476,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( // delete the discriminator field to simulate a consistency update that causes the field // to be missing for a record environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isActor')).toBe(true); - record.setValue(undefined, '__isActor'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(true); + typeRecord.setValue(undefined, '__isActor'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -478,11 +498,21 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(abstractOperation).status).toBe('missing'); expect(environment.check(concreteOperation).status).toBe('missing'); expect(environment.check(operation).status).toBe('missing'); + + // Subscriptions are not notified of discriminator-only changes + const callback = jest.fn(); + environment.subscribe(fragmentSnapshot, callback); + environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(undefined); + typeRecord.setValue(false, '__isActor'); + }); + expect(callback).toBeCalledTimes(0); }); it('abstract inline fragment on implementing type reads data and counts missing user fields as missing', () => { // with missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -510,7 +540,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(operation).status).toBe('missing'); // add missing value - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, @@ -538,7 +568,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); it('abstract inline fragment on non-implementing type reads data but does not count missing user fields as missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', // __isActor: 'User', // no value: means that on server, User no longer implements Actor @@ -568,7 +598,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); it('abstract inline fragment missing only the discriminator reads data and counts data as missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, // deleted from store below @@ -581,9 +611,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( // delete the discriminator field to simulate a consistency update that causes the field // to be missing for a record environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isActor')).toBe(true); - record.setValue(undefined, '__isActor'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(true); + typeRecord.setValue(undefined, '__isActor'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -601,10 +631,20 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); expect(fragmentSnapshot.isMissingData).toBe(true); expect(environment.check(operation).status).toBe('missing'); + + // Subscriptions are not notified of discriminator-only changes + const callback = jest.fn(); + environment.subscribe(fragmentSnapshot, callback); + environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(undefined); + typeRecord.setValue(false, '__isActor'); + }); + expect(callback).toBeCalledTimes(0); }); it('abstract inline fragment missing the discriminator and user fields: reads data and counts data as missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { id: 'abc', __isActor: true, // deleted from store below @@ -617,9 +657,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( // delete the discriminator field to simulate a consistency update that causes the field // to be missing for a record environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isActor')).toBe(true); - record.setValue(undefined, '__isActor'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(true); + typeRecord.setValue(undefined, '__isActor'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -639,6 +679,16 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( expect(environment.check(abstractOperation).status).toBe('missing'); expect(environment.check(concreteOperation).status).toBe('missing'); expect(environment.check(operation).status).toBe('missing'); + + // Subscriptions are not notified of discriminator-only changes + const callback = jest.fn(); + environment.subscribe(fragmentSnapshot, callback); + environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(undefined); + typeRecord.setValue(false, '__isActor'); + }); + expect(callback).toBeCalledTimes(0); }); describe('abstract spreads within a field of matching abstract type', () => { @@ -669,7 +719,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); it('reads and reports missing data if only user fields are missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { viewer: { actor: { __typename: 'User', @@ -708,7 +758,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); it('reads and reports missing data if only the discriminator is missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { viewer: { actor: { __typename: 'User', @@ -722,9 +772,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( // delete the discriminator field to simulate a consistency update that causes the field // to be missing for a record environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isActor')).toBe(true); - record.setValue(undefined, '__isActor'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(true); + typeRecord.setValue(undefined, '__isActor'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -754,7 +804,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); it('reads and reports missing data if the discriminator and user fields are missing', () => { - environment.commitPayload(operation, { + commitPayload(operation, { viewer: { actor: { __typename: 'User', @@ -768,9 +818,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( // delete the discriminator field to simulate a consistency update that causes the field // to be missing for a record environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isActor')).toBe(true); - record.setValue(undefined, '__isActor'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isActor')).toBe(true); + typeRecord.setValue(undefined, '__isActor'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -827,7 +877,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing even if the type discriminator and user fields are missing', () => { // typical case, server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -866,7 +916,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if only user fields are missing', () => { // similar case, we know somehow that the record implements the nested abstract type, but // the fields are missing since the server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -877,9 +927,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the discriminator environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isEntity')).toBe(undefined); - record.setValue(true, '__isEntity'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isEntity')).toBe(undefined); + typeRecord.setValue(true, '__isEntity'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -911,7 +961,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if only the type discriminator is missing', () => { // the fields from the nested spread were fetched elsewhere in the query, but we're missing the refinement // typical case, server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -922,8 +972,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the missing user field environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isEntity')).toBe(undefined); const record = nullthrows(store.get('abc')); - expect(record.getValue('__isEntity')).toBe(undefined); record.setValue('https://...', 'url'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); @@ -955,7 +1006,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if the discriminator and all fields are present', () => { // somehow we have all the data - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -966,9 +1017,10 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the missing user field *and* discriminator environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isEntity')).toBe(undefined); + typeRecord.setValue(true, '__isEntity'); const record = nullthrows(store.get('abc')); - expect(record.getValue('__isEntity')).toBe(undefined); - record.setValue(true, '__isEntity'); record.setValue('https://...', 'url'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); @@ -1026,7 +1078,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing even if the type discriminator and user fields are missing', () => { // typical case, server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1066,7 +1118,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if only user fields are missing', () => { // similar case, we know somehow that the record implements the nested abstract type, but // the fields are missing since the server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1078,9 +1130,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the discriminator environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isNamed')).toBe(undefined); - record.setValue(true, '__isNamed'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isNamed')).toBe(undefined); + typeRecord.setValue(true, '__isNamed'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -1112,7 +1164,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if only the type discriminator is missing', () => { // the fields from the nested spread were fetched elsewhere in the query, but we're missing the refinement // typical case, server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1124,8 +1176,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the missing user field environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isNamed')).toBe(undefined); const record = nullthrows(store.get('abc')); - expect(record.getValue('__isNamed')).toBe(undefined); record.setValue('Zuck', 'name'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); @@ -1157,7 +1210,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if the discriminator and all fields are present', () => { // somehow we have all the data - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1169,9 +1222,10 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the missing user field *and* discriminator environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isNamed')).toBe(undefined); + typeRecord.setValue(true, '__isNamed'); const record = nullthrows(store.get('abc')); - expect(record.getValue('__isNamed')).toBe(undefined); - record.setValue(true, '__isNamed'); record.setValue('Zuck', 'name'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); @@ -1231,7 +1285,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing even if the type discriminator and user fields are missing', () => { // typical case, server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1271,7 +1325,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if only user fields are missing', () => { // similar case, we know somehow that the record implements the nested abstract type, but // the fields are missing since the server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1283,9 +1337,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the discriminator environment.commitUpdate(store => { - const record = nullthrows(store.get('abc')); - expect(record.getValue('__isNamed')).toBe(undefined); - record.setValue(true, '__isNamed'); + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isNamed')).toBe(undefined); + typeRecord.setValue(true, '__isNamed'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); const fragmentSnapshot = environment.lookup( @@ -1317,7 +1371,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if only the type discriminator is missing', () => { // the fields from the nested spread were fetched elsewhere in the query, but we're missing the refinement // typical case, server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1329,8 +1383,9 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the missing user field environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isNamed')).toBe(undefined); const record = nullthrows(store.get('abc')); - expect(record.getValue('__isNamed')).toBe(undefined); record.setValue('Zuck', 'name'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); @@ -1362,7 +1417,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if the discriminator and all fields are present', () => { // somehow we have all the data - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1374,9 +1429,10 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( }); // consistency update that provides the missing user field *and* discriminator environment.commitUpdate(store => { + const typeRecord = nullthrows(store.get(generateTypeID('User'))); + expect(typeRecord.getValue('__isNamed')).toBe(undefined); + typeRecord.setValue(true, '__isNamed'); const record = nullthrows(store.get('abc')); - expect(record.getValue('__isNamed')).toBe(undefined); - record.setValue(true, '__isNamed'); record.setValue('Zuck', 'name'); }); const parentSnapshot: $FlowFixMe = environment.lookup(operation.fragment); @@ -1434,7 +1490,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing even if user fields are missing', () => { // typical case, server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1473,7 +1529,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if all fields are present', () => { // somehow we have all the data - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1545,7 +1601,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing even if user fields are missing', () => { // typical case, server doesn't evaluate anything under the non-matched parent - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment @@ -1584,7 +1640,7 @@ describe('missing data detection with feature ENABLE_PRECISE_TYPE_REFINEMENT', ( it('reads data and reports nothing missing if all fields are present', () => { // somehow we have all the data - environment.commitPayload(operation, { + commitPayload(operation, { userOrPage: { __typename: 'User', __isNode: 'User', // selected by the auto-generated `... on Node { id }` fragment diff --git a/packages/relay-runtime/store/__tests__/RelayReader-test.js b/packages/relay-runtime/store/__tests__/RelayReader-test.js index 40873cb0e2ef1..c602518a84279 100644 --- a/packages/relay-runtime/store/__tests__/RelayReader-test.js +++ b/packages/relay-runtime/store/__tests__/RelayReader-test.js @@ -10,6 +10,7 @@ 'use strict'; +const RelayFeatureFlags = require('../../util/RelayFeatureFlags'); const RelayRecordSource = require('../RelayRecordSource'); const {getRequest} = require('../../query/GraphQLTag'); @@ -19,6 +20,7 @@ const { const {createReaderSelector} = require('../RelayModernSelector'); const {read} = require('../RelayReader'); const {ROOT_ID} = require('../RelayStoreUtils'); +const {generateTypeID, TYPE_SCHEMA_TYPE} = require('../TypeID'); const {generateAndCompile} = require('relay-test-utils-internal'); describe('RelayReader', () => { @@ -1722,4 +1724,68 @@ describe('RelayReader', () => { }); }); }); + + describe('feature ENABLE_PRECISE_TYPE_REFINEMENT', () => { + beforeEach(() => { + RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT = true; + }); + afterEach(() => { + RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT = false; + }); + + it('does not record a dependency on type records for abstract type discriminators', () => { + const {Query, Fragment} = generateAndCompile(` + query Query { + me { + ...Fragment + } + } + fragment Fragment on Node { + actor { + ... on Entity { + url + } + } + } + `); + const userTypeID = generateTypeID('User'); + const pageTypeID = generateTypeID('Page'); + const data = { + '1': { + __id: '1', + __typename: 'User', + actor: {__ref: '2'}, + }, + '2': { + __id: '2', + __typename: 'Page', + url: 'https://...', + }, + [userTypeID]: { + __id: userTypeID, + __typename: TYPE_SCHEMA_TYPE, + __isNode: true, + }, + [pageTypeID]: { + __id: pageTypeID, + __typename: TYPE_SCHEMA_TYPE, + // __isEntity: true, // intentionally missing to verify that type refinement feature is on + }, + }; + source = RelayRecordSource.create(data); + const owner = createOperationDescriptor(Query, {}); + const snapshot = read( + source, + createReaderSelector(Fragment, '1', {}, owner.request), + ); + expect(snapshot.data).toEqual({ + actor: { + url: 'https://...', + }, + }); + expect(snapshot.isMissingData).toBe(true); // missing discriminator + // does *not* include userTypeID/pageTypeID + expect(Object.keys(snapshot.seenRecords)).toEqual(['1', '2']); + }); + }); });