Skip to content

Commit

Permalink
Type refinement uses per-type metadata (previously per-record)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
josephsavona authored and facebook-github-bot committed May 26, 2020
1 parent e913579 commit 2cf9ac9
Show file tree
Hide file tree
Showing 8 changed files with 295 additions and 86 deletions.
19 changes: 17 additions & 2 deletions packages/relay-runtime/store/DataChecker.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const invariant = require('invariant');

const {isClientID} = require('./ClientID');
const {EXISTENT, UNKNOWN} = require('./RelayRecordState');
const {generateTypeID} = require('./TypeID');

import type {
NormalizationField,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
24 changes: 16 additions & 8 deletions packages/relay-runtime/store/RelayReader.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const {
getStorageKey,
getModuleComponentKey,
} = require('./RelayStoreUtils');
const {generateTypeID} = require('./TypeID');

import type {
ReaderFragmentSpread,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
17 changes: 16 additions & 1 deletion packages/relay-runtime/store/RelayReferenceMarker.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
Expand Down
19 changes: 17 additions & 2 deletions packages/relay-runtime/store/RelayResponseNormalizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
);
Expand All @@ -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,
);
Expand Down
28 changes: 28 additions & 0 deletions packages/relay-runtime/store/TypeID.js
Original file line number Diff line number Diff line change
@@ -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};
8 changes: 7 additions & 1 deletion packages/relay-runtime/store/__tests__/DataChecker-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -2204,6 +2205,7 @@ describe('check()', () => {
}
}
`);
const typeID = generateTypeID('NonNodeNoID');
const data = {
'client:root': {
__id: 'client:root',
Expand All @@ -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();
Expand Down
Loading

0 comments on commit 2cf9ac9

Please sign in to comment.