Skip to content

Commit

Permalink
Stop using shared DeepMerger in ReadContext for executeSelectionSet.
Browse files Browse the repository at this point in the history
Issue #9735 appears to be due to a bug I introduced in my commit
756ab87 in PR #8734.

Premature optimization has a bad reputation already, but you can add
this PR to the mountain of evidence in its disfavor.
  • Loading branch information
benjamn committed May 19, 2022
1 parent 964ca12 commit 466519f
Showing 1 changed file with 11 additions and 19 deletions.
30 changes: 11 additions & 19 deletions src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ import {
getFragmentDefinitions,
getMainDefinition,
getQueryDefinition,
DeepMerger,
getFragmentFromSelection,
maybeDeepFreeze,
mergeDeepArray,
DeepMerger,
isNonNullObject,
canUseWeakMap,
compact,
Expand All @@ -49,8 +50,6 @@ interface ReadContext extends ReadMergeModifyContext {
policies: Policies;
canonizeResults: boolean;
fragmentMap: FragmentMap;
// General-purpose deep-merge function for use during reads.
merge<T>(existing: T, incoming: T): T;
};

export type ExecResult<R = any> = {
Expand Down Expand Up @@ -233,7 +232,6 @@ export class StoreReader {
};

const rootRef = makeReference(rootId);
const merger = new DeepMerger;
const execResult = this.executeSelectionSet({
selectionSet: getMainDefinition(query).selectionSet,
objectOrReference: rootRef,
Expand All @@ -246,15 +244,6 @@ export class StoreReader {
varString: canonicalStringify(variables),
canonizeResults,
fragmentMap: createFragmentMap(getFragmentDefinitions(query)),
merge(a, b) {
// We use the same DeepMerger instance throughout the read, so any
// merged objects created during this read can be updated later in the
// read using in-place/destructive property assignments. Once the read
// is finished, these objects will be frozen, but in the meantime it's
// good for performance and memory usage if we avoid allocating a new
// object for every merged property.
return merger.merge(a, b);
},
},
});

Expand Down Expand Up @@ -325,21 +314,22 @@ export class StoreReader {
const { variables, policies, store } = context;
const typename = store.getFieldValue<string>(objectOrReference, "__typename");

let result: any = {};
const objectsToMerge: Record<string, any>[] = [];
let missing: MissingTree | undefined;
const missingMerger = new DeepMerger();

if (this.config.addTypename &&
typeof typename === "string" &&
!policies.rootIdsByTypename[typename]) {
// Ensure we always include a default value for the __typename
// field, if we have one, and this.config.addTypename is true. Note
// that this field can be overridden by other merged objects.
result = { __typename: typename };
objectsToMerge.push({ __typename: typename });
}

function handleMissing<T>(result: ExecResult<T>, resultName: string): T {
if (result.missing) {
missing = context.merge(missing, { [resultName]: result.missing });
missing = missingMerger.merge(missing, { [resultName]: result.missing });
}
return result.result;
}
Expand All @@ -363,7 +353,7 @@ export class StoreReader {

if (fieldValue === void 0) {
if (!addTypenameToDocument.added(selection)) {
missing = context.merge(missing, {
missing = missingMerger.merge(missing, {
[resultName]: `Can't find field '${
selection.name.value
}' on ${
Expand Down Expand Up @@ -404,7 +394,7 @@ export class StoreReader {
}

if (fieldValue !== void 0) {
result = context.merge(result, { [resultName]: fieldValue });
objectsToMerge.push({ [resultName]: fieldValue });
}

} else {
Expand All @@ -419,6 +409,7 @@ export class StoreReader {
}
});

const result = mergeDeepArray(objectsToMerge);
const finalResult: ExecResult = { result, missing };
const frozen = context.canonizeResults
? this.canon.admit(finalResult)
Expand All @@ -443,10 +434,11 @@ export class StoreReader {
context,
}: ExecSubSelectedArrayOptions): ExecResult {
let missing: MissingTree | undefined;
let missingMerger = new DeepMerger<MissingTree[]>();

function handleMissing<T>(childResult: ExecResult<T>, i: number): T {
if (childResult.missing) {
missing = context.merge(missing, { [i]: childResult.missing });
missing = missingMerger.merge(missing, { [i]: childResult.missing });
}
return childResult.result;
}
Expand Down

0 comments on commit 466519f

Please sign in to comment.