From f1cda57bb29c2f2cb3ef07d31e8c0217f493f548 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 18 May 2021 12:54:40 -0400 Subject: [PATCH] Restrict ObjectCanon prototypes to {Array,Object}.prototype and null. As promised in https://github.com/apollographql/apollo-client/pull/8222#discussion_r633891466, a more general implementation of ObjectCanon is coming, so I think we should restrict the functionality of the current implementation to match what we have planned. First, I want to reserve the isCanonical method to return true for anything that doesn't need to be (further) canonized, including primitive values. To that end, the isKnown method now returns true only for *objects* previously returned by canon.admit. Second, the future implemenation will allow full customization of canonization by object prototype, but will only canonize arrays and plain objects by default. To that end, I've resricted ObjectCanon canonization to objects whose prototypes are Array.prototype, Object.prototype, or null. --- src/cache/inmemory/__tests__/object-canon.ts | 4 +++- src/cache/inmemory/__tests__/readFromStore.ts | 12 ++++++------ src/cache/inmemory/helpers.ts | 1 - src/cache/inmemory/object-canon.ts | 11 ++++++----- src/cache/inmemory/readFromStore.ts | 2 +- 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/cache/inmemory/__tests__/object-canon.ts b/src/cache/inmemory/__tests__/object-canon.ts index 83014a7f3bb..8af77d614cf 100644 --- a/src/cache/inmemory/__tests__/object-canon.ts +++ b/src/cache/inmemory/__tests__/object-canon.ts @@ -46,7 +46,9 @@ describe("ObjectCanon", () => { expect(canon.admit(c2)).toBe(c2); }); - it("preserves custom prototypes", () => { + // TODO Reenable this when ObjectCanon allows enabling canonization for + // arbitrary prototypes (not just {Array,Object}.prototype and null). + it.skip("preserves custom prototypes", () => { const canon = new ObjectCanon; class Custom { diff --git a/src/cache/inmemory/__tests__/readFromStore.ts b/src/cache/inmemory/__tests__/readFromStore.ts index 3df74615432..54d9e691741 100644 --- a/src/cache/inmemory/__tests__/readFromStore.ts +++ b/src/cache/inmemory/__tests__/readFromStore.ts @@ -2044,11 +2044,11 @@ describe('reading from the store', () => { } const nonCanonicalQueryResult0 = readQuery(false); - expect(canon.isCanonical(nonCanonicalQueryResult0)).toBe(false); + expect(canon.isKnown(nonCanonicalQueryResult0)).toBe(false); expect(nonCanonicalQueryResult0).toEqual({ count: 0 }); const canonicalQueryResult0 = readQuery(true); - expect(canon.isCanonical(canonicalQueryResult0)).toBe(true); + expect(canon.isKnown(canonicalQueryResult0)).toBe(true); // The preservation of { count: 0 } proves the result didn't have to be // recomputed, but merely canonized. expect(canonicalQueryResult0).toEqual({ count: 0 }); @@ -2058,7 +2058,7 @@ describe('reading from the store', () => { }); const canonicalQueryResult1 = readQuery(true); - expect(canon.isCanonical(canonicalQueryResult1)).toBe(true); + expect(canon.isKnown(canonicalQueryResult1)).toBe(true); expect(canonicalQueryResult1).toEqual({ count: 1 }); const nonCanonicalQueryResult1 = readQuery(false); @@ -2101,7 +2101,7 @@ describe('reading from the store', () => { } const canonicalFragmentResult1 = readFragment(true); - expect(canon.isCanonical(canonicalFragmentResult1)).toBe(true); + expect(canon.isKnown(canonicalFragmentResult1)).toBe(true); expect(canonicalFragmentResult1).toEqual({ count: 0 }); const nonCanonicalFragmentResult1 = readFragment(false); @@ -2115,13 +2115,13 @@ describe('reading from the store', () => { const nonCanonicalFragmentResult2 = readFragment(false); expect(readFragment(false)).toBe(nonCanonicalFragmentResult2); - expect(canon.isCanonical(nonCanonicalFragmentResult2)).toBe(false); + expect(canon.isKnown(nonCanonicalFragmentResult2)).toBe(false); expect(nonCanonicalFragmentResult2).toEqual({ count: 1 }); expect(readFragment(false)).toBe(nonCanonicalFragmentResult2); const canonicalFragmentResult2 = readFragment(true); expect(readFragment(true)).toBe(canonicalFragmentResult2); - expect(canon.isCanonical(canonicalFragmentResult2)).toBe(true); + expect(canon.isKnown(canonicalFragmentResult2)).toBe(true); expect(canonicalFragmentResult2).toEqual({ count: 1 }); }); }); diff --git a/src/cache/inmemory/helpers.ts b/src/cache/inmemory/helpers.ts index 9777c8330a0..b4627cab30b 100644 --- a/src/cache/inmemory/helpers.ts +++ b/src/cache/inmemory/helpers.ts @@ -14,7 +14,6 @@ import { export const { hasOwnProperty: hasOwn, - toString: objToStr, } = Object.prototype; export function getTypenameFromStoreObject( diff --git a/src/cache/inmemory/object-canon.ts b/src/cache/inmemory/object-canon.ts index d9a2747e2a7..7c76971d220 100644 --- a/src/cache/inmemory/object-canon.ts +++ b/src/cache/inmemory/object-canon.ts @@ -1,6 +1,5 @@ import { Trie } from "@wry/trie"; import { canUseWeakMap } from "../../utilities"; -import { objToStr } from "./helpers"; function isObjectOrArray(value: any): value is object { return !!value && typeof value === "object"; @@ -82,7 +81,7 @@ export class ObjectCanon { keys?: SortedKeysInfo; }>(canUseWeakMap); - public isCanonical(value: any): boolean { + public isKnown(value: any): boolean { return isObjectOrArray(value) && this.known.has(value); } @@ -106,8 +105,9 @@ export class ObjectCanon { const original = this.passes.get(value); if (original) return original; - switch (objToStr.call(value)) { - case "[object Array]": { + const proto = Object.getPrototypeOf(value); + switch (proto) { + case Array.prototype: { if (this.known.has(value)) return value; const array: any[] = (value as any[]).map(this.admit, this); // Arrays are looked up in the Trie using their recursively @@ -126,7 +126,8 @@ export class ObjectCanon { return node.array; } - case "[object Object]": { + case null: + case Object.prototype: { if (this.known.has(value)) return value; const proto = Object.getPrototypeOf(value); const array = [proto]; diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index 2572c6c0607..803855c7526 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -273,7 +273,7 @@ export class StoreReader { // If result is canonical, then it could only have been previously // cached by the canonizing version of executeSelectionSet, so we can // avoid checking both possibilities here. - this.canon.isCanonical(result), + this.canon.isKnown(result), ); if (latest && result === latest.result) { return true;