From 6d1d05ff91507ea844f332f65f5bc15cc8e10655 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 5 Jun 2023 14:46:59 -0400 Subject: [PATCH] [runtime] Update makeReadOnly runtime --- (wip, waiting for feedback on workplace post) - remove calls to `isInROMode`, as we want to log all mutations after 'freezing' a value (both within and outside of render cycle) - add `source` parameter -- this is the function name of the parent component / hook --- .../src/makeReadOnly.test.ts | 105 +++++++++++------- .../make-read-only-util/src/makeReadOnly.ts | 44 ++++---- 2 files changed, 91 insertions(+), 58 deletions(-) diff --git a/compiler/forget/packages/make-read-only-util/src/makeReadOnly.test.ts b/compiler/forget/packages/make-read-only-util/src/makeReadOnly.test.ts index 9734fb0a74..4ac9c7e3d3 100644 --- a/compiler/forget/packages/make-read-only-util/src/makeReadOnly.test.ts +++ b/compiler/forget/packages/make-read-only-util/src/makeReadOnly.test.ts @@ -9,11 +9,11 @@ import buildMakeReadOnly from "./makeReadOnly"; describe("makeReadOnly", () => { let logger: jest.Func; - let makeReadOnly: (value: T) => T; + let makeReadOnly: (value: T, source: string) => T; beforeEach(() => { logger = jest.fn(); - makeReadOnly = buildMakeReadOnly(logger, [], () => true); + makeReadOnly = buildMakeReadOnly(logger, []); }); describe("Tracking mutations", () => { @@ -21,45 +21,50 @@ describe("makeReadOnly", () => { const a = 5; const b = true; const c = null; - expect(makeReadOnly(a)).toBe(a); - expect(makeReadOnly(b)).toBe(b); - expect(makeReadOnly(c)).toBe(c); + expect(makeReadOnly(a, "test1")).toBe(a); + expect(makeReadOnly(b, "test1")).toBe(b); + expect(makeReadOnly(c, "test1")).toBe(c); }); it("retains referential equality", () => { const valA = {}; const valB = { a: valA, _: valA }; const o = { a: valA, b: valB, c: "c" }; - expect(makeReadOnly(o)).toBe(o); - expect(makeReadOnly(o.a)).toBe(valA); - expect(makeReadOnly(o.b)).toBe(valB); - expect(makeReadOnly(o.b.a)).toBe(valA); - expect(makeReadOnly(o.b._)).toBe(valA); - expect(makeReadOnly(o.c)).toBe("c"); + expect(makeReadOnly(o, "test2")).toBe(o); + expect(makeReadOnly(o.a, "test2")).toBe(valA); + expect(makeReadOnly(o.b, "test2")).toBe(valB); + expect(makeReadOnly(o.b.a, "test2")).toBe(valA); + expect(makeReadOnly(o.b._, "test2")).toBe(valA); + expect(makeReadOnly(o.c, "test2")).toBe("c"); }); it("deals with cyclic references", () => { const o: any = {}; o.self_ref = o; - expect(makeReadOnly(o)).toBe(o); - expect(makeReadOnly(o.self_ref)).toBe(o); + expect(makeReadOnly(o, "test3")).toBe(o); + expect(makeReadOnly(o.self_ref, "test3")).toBe(o); }); it("logs direct interior mutability", () => { const o = { a: 0 }; - makeReadOnly(o); + makeReadOnly(o, "test4"); o.a = 42; - expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "a", 42); + expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "test4", "a", 42); }); it("tracks changes to known RO properties", () => { const o: any = { a: {} }; - makeReadOnly(o); + makeReadOnly(o, "test5"); o.a = 42; - expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "a", 42); + expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "test5", "a", 42); expect(o.a).toBe(42); const newVal = { x: 0 }; o.a = newVal; - expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "a", newVal); + expect(logger).toBeCalledWith( + "FORGET_MUTATE_IMMUT", + "test5", + "a", + newVal + ); expect(o.a).toBe(newVal); }); @@ -67,18 +72,23 @@ describe("makeReadOnly", () => { const o: any = { a: { x: 4 } }; const alias = o; - makeReadOnly(o); + makeReadOnly(o, "test6"); const newVal = {}; alias.a = newVal; - expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "a", newVal); + expect(logger).toBeCalledWith( + "FORGET_MUTATE_IMMUT", + "test6", + "a", + newVal + ); expect(o.a).toBe(newVal); }); it("logs transitive interior mutability", () => { const o: any = { a: { x: 0 } }; - makeReadOnly(o); + makeReadOnly(o, "test7"); o.a.x = 42; - expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "x", 42); + expect(logger).toBeCalledWith("FORGET_MUTATE_IMMUT", "test7", "x", 42); }); describe("todo", () => { @@ -86,7 +96,7 @@ describe("makeReadOnly", () => { // this is a limitation of the current "proxy" approach, // which overwrites object properties with getters and setters const x: any = { a: {} }; - makeReadOnly(x); + makeReadOnly(x, "test8"); delete x.a; x.b = 0; @@ -96,44 +106,61 @@ describe("makeReadOnly", () => { // this could be easily implemented by making caching eager const innerObj = { x: 0 }; const o = { a: innerObj }; - makeReadOnly(o); + makeReadOnly(o, "test9"); innerObj.x = 42; expect(o.a.x).toBe(42); const o1 = { a: { x: 0 } }; const innerObj1 = o1.a; - makeReadOnly(o1); + makeReadOnly(o1, "test9"); innerObj1.x = 42; expect(o1.a.x).toBe(42); expect(logger).toBeCalledTimes(0); }); + + it("does not track objects with getter/setters", () => { + let backedX: string | null = null; + const o = { + set val(val: string | null) { + backedX = val; + }, + get val(): string | null { + return backedX; + }, + }; + expect(makeReadOnly(o, "test10")).toBe(o); + expect(makeReadOnly(o.val, "test10")).toBe(null); + + o.val = "40"; + expect(logger).toBeCalledTimes(0); + }); }); }); describe("Tracking adding or deleting properties", () => { it("tracks new properties added between calls to makeReadOnly", () => { const o: any = {}; - makeReadOnly(o); + makeReadOnly(o, "test11"); o.a = "new value"; - makeReadOnly(o); - expect(logger).toBeCalledWith("FORGET_ADD_PROP_IMMUT", "a"); + makeReadOnly(o, "test11"); + expect(logger).toBeCalledWith("FORGET_ADD_PROP_IMMUT", "test11", "a"); }); it("tracks properties deleted between calls to makeReadOnly", () => { const o: any = { a: 0 }; - makeReadOnly(o); + makeReadOnly(o, "test12"); delete o.a; - makeReadOnly(o); - expect(logger).toBeCalledWith("FORGET_DELETE_PROP_IMMUT", "a"); + makeReadOnly(o, "test12"); + expect(logger).toBeCalledWith("FORGET_DELETE_PROP_IMMUT", "test12", "a"); }); - it("tracks properties deleted and re-added between calls to makeReadOnly", () => { - const o: any = { a: 0 }; - makeReadOnly(o); - delete o.a; - o.a = {}; - makeReadOnly(o); - expect(logger).toBeCalledWith("FORGET_CHANGE_PROP_IMMUT", "a"); - }); + // it("tracks properties deleted and re-added between calls to makeReadOnly", () => { + // const o: any = { a: 0 }; + // makeReadOnly(o); + // delete o.a; + // o.a = {}; + // makeReadOnly(o); + // expect(logger).toBeCalledWith("FORGET_CHANGE_PROP_IMMUT", "a"); + // }); }); }); diff --git a/compiler/forget/packages/make-read-only-util/src/makeReadOnly.ts b/compiler/forget/packages/make-read-only-util/src/makeReadOnly.ts index 25d0e94690..a9df3bacdb 100644 --- a/compiler/forget/packages/make-read-only-util/src/makeReadOnly.ts +++ b/compiler/forget/packages/make-read-only-util/src/makeReadOnly.ts @@ -12,9 +12,9 @@ type ROViolationType = | "FORGET_DELETE_PROP_IMMUT" | "FORGET_CHANGE_PROP_IMMUT" | "FORGET_ADD_PROP_IMMUT"; -type ROModeChecker = () => boolean; type ROViolationLogger = ( violation: ROViolationType, + source: string, key: string, value?: any ) => void; @@ -52,15 +52,15 @@ function getOrInsertDefault( function buildMakeReadOnly( logger: ROViolationLogger, - skippedClasses: string[], - isInROMode: ROModeChecker -): (val: T) => T { + skippedClasses: string[] +): (val: T, source: string) => T { // All saved proxys const savedROObjects: SavedROObjects = new WeakMap(); // Overwrites an object property with its proxy and saves its original value function addProperty( obj: Object, + source: string, key: string, prop: PropertyDescriptor, savedEntries: Map @@ -68,12 +68,10 @@ function buildMakeReadOnly( const proxy: PropertyDescriptor & { get(): unknown } = { get() { // read from backing cache entry - return makeReadOnly(savedEntries.get(key)!.savedVal); + return makeReadOnly(savedEntries.get(key)!.savedVal, source); }, set(newVal: unknown) { - if (isInROMode()) { - logger("FORGET_MUTATE_IMMUT", key, newVal); - } + logger("FORGET_MUTATE_IMMUT", source, key, newVal); // update backing cache entry savedEntries.get(key)!.savedVal = newVal; }, @@ -90,10 +88,13 @@ function buildMakeReadOnly( } // Changes an object to be read-only, returns its input - function makeReadOnly(o: T): T { + function makeReadOnly(o: T, source: string): T { if (typeof o !== "object" || o == null) { return o; - } else if (skippedClasses.includes(o.constructor.name)) { + } else if ( + o.constructor?.name != null && + skippedClasses.includes(o.constructor.name) + ) { return o; } @@ -114,13 +115,11 @@ function buildMakeReadOnly( // (meaning that new value is not proxied, // and the current proxied value is stale) cache.delete(k); - if (!currentProp && isInROMode()) { - logger("FORGET_DELETE_PROP_IMMUT", k); + if (!currentProp) { + logger("FORGET_DELETE_PROP_IMMUT", source, k); } else if (currentProp) { - if (isInROMode()) { - logger("FORGET_CHANGE_PROP_IMMUT", k); - } - addProperty(o, k, currentProp, cache); + logger("FORGET_CHANGE_PROP_IMMUT", source, k); + addProperty(o, source, k, currentProp, cache); } } } @@ -128,10 +127,17 @@ function buildMakeReadOnly( Object.getOwnPropertyDescriptors(o) )) { if (!cache.has(k) && isWriteable(prop)) { - if (isInROMode() && existed) { - logger("FORGET_ADD_PROP_IMMUT", k); + if (prop.hasOwnProperty("set") || prop.hasOwnProperty("get") || k === "current") { + // - we currently don't handle accessor properties + // - we currently have no other way of checking whether an object + // is a `ref` (i.e. returned by useRef). + continue; } - addProperty(o, k, prop, cache); + + if (existed) { + logger("FORGET_ADD_PROP_IMMUT", source, k); + } + addProperty(o, source, k, prop, cache); } } return o;