From f6e8aff72597737b433edb8100b97f27d8da86ba Mon Sep 17 00:00:00 2001 From: Samuel Degueldre Date: Tue, 29 Nov 2022 10:03:08 +0100 Subject: [PATCH] [FIX] reactivity: fix issues with reactive objects in proto chain Previously, when there was a reactive object in the prototype chain of a different object, it would get notified of the first write to a property that existed on the reactive object but did not yet exist on the other object, despite the value of that property not actually getting written to the reactive and hence the value not getting changed. This was caused by the fact that we assumed that Reflect.set would set the value on the target, when in fact, the value is set on the object that underlies the `receiver`. When a reactive object is part of the prototype chain, the first write on a key triggers the set trap but the receiver is not the reactive object, and so Reflect.set doesn't modify the target, but adds the new key to the object that's lower in the prototype chain. This commit fixes that by actually reading the value from the target instead of assuming that it was changed by Reflect.set This commit also removes the special symbols SKIP and TARGET, as there is no reliable way to check whether these keys are present on the object itself or on its prototype chain, which can cause issue when trying to create reactive objects from objects with other reactive objects in their prototype chain, or to create reactive objects from objects with non-reactive objects in their prototype chain. To solve this problem, we simply use a WeakMap that maps reactives to their targets, and a WeakSet that contains all skipped objects. --- src/runtime/component_node.ts | 15 ++----- src/runtime/index.ts | 1 - src/runtime/reactivity.ts | 78 +++++++++++++++-------------------- tests/reactivity.test.ts | 46 +++++++++++++++++++-- 4 files changed, 81 insertions(+), 59 deletions(-) diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts index de925700..62babc23 100644 --- a/src/runtime/component_node.ts +++ b/src/runtime/component_node.ts @@ -3,14 +3,7 @@ import { BDom, VNode } from "./blockdom"; import { Component, ComponentConstructor, Props } from "./component"; import { fibersInError, OwlError } from "./error_handling"; import { Fiber, makeChildFiber, makeRootFiber, MountFiber, MountOptions } from "./fibers"; -import { - clearReactivesForCallback, - getSubscriptions, - NonReactive, - Reactive, - reactive, - TARGET, -} from "./reactivity"; +import { clearReactivesForCallback, getSubscriptions, reactive, targets } from "./reactivity"; import { STATUS } from "./status"; import { batched, Callback } from "./utils"; @@ -52,7 +45,7 @@ const batchedRenderFunctions = new WeakMap(); * relevant changes * @see reactive */ -export function useState(state: T): Reactive | NonReactive { +export function useState(state: T): T { const node = getCurrent(); let render = batchedRenderFunctions.get(node)!; if (!render) { @@ -116,7 +109,7 @@ export class ComponentNode

implements VNode implements VNode = T; + type Collection = Set | Map | WeakMap; type CollectionRawType = "Set" | "Map" | "WeakMap"; -export type Reactive = T & { - [TARGET]: any; -}; - -export type NonReactive = T & { - [SKIP]: any; -}; - const objectToString = Object.prototype.toString; const objectHasOwnProperty = Object.prototype.hasOwnProperty; @@ -61,15 +53,16 @@ function possiblyReactive(val: any, cb: Callback) { return canBeMadeReactive(val) ? reactive(val, cb) : val; } +const skipped = new WeakSet(); /** * Mark an object or array so that it is ignored by the reactivity system * * @param value the value to mark * @returns the object itself */ -export function markRaw(value: T): NonReactive { - (value as any)[SKIP] = true; - return value as NonReactive; +export function markRaw(value: T): T { + skipped.add(value); + return value; } /** @@ -78,8 +71,8 @@ export function markRaw(value: T): NonReactive { * @param value a reactive value * @returns the underlying value */ -export function toRaw(value: Reactive): T { - return value[TARGET] || value; +export function toRaw>(value: U | T): T { + return targets.has(value) ? (targets.get(value) as T) : value; } const targetToKeysToCallbacks = new WeakMap>>(); @@ -164,8 +157,9 @@ export function getSubscriptions(callback: Callback) { }; }); } - -const reactiveCache = new WeakMap>(); +// Maps reactive objects to the underlying target +export const targets = new WeakMap, Target>(); +const reactiveCache = new WeakMap>>(); /** * Creates a reactive proxy for an object. Reading data on the reactive object * subscribes to changes to the data. Writing data on the object will cause the @@ -180,7 +174,7 @@ const reactiveCache = new WeakMap>(); * Subscriptions: * + Reading a property on an object will subscribe you to changes in the value * of that property. - * + Accessing an object keys (eg with Object.keys or with `for..in`) will + * + Accessing an object's keys (eg with Object.keys or with `for..in`) will * subscribe you to the creation/deletion of keys. Checking the presence of a * key on the object with 'in' has the same effect. * - getOwnPropertyDescriptor does not currently subscribe you to the property. @@ -193,19 +187,16 @@ const reactiveCache = new WeakMap>(); * reactive has changed * @returns a proxy that tracks changes to it */ -export function reactive( - target: T, - callback: Callback = () => {} -): Reactive | NonReactive { +export function reactive(target: T, callback: Callback = () => {}): T { if (!canBeMadeReactive(target)) { throw new OwlError(`Cannot make the given value reactive`); } - if (SKIP in target) { - return target as NonReactive; + if (skipped.has(target)) { + return target; } - const originalTarget = (target as Reactive)[TARGET]; - if (originalTarget) { - return reactive(originalTarget, callback); + if (targets.has(target)) { + // target is reactive, create a reactive on the underlying object instead + return reactive(targets.get(target) as T, callback); } if (!reactiveCache.has(target)) { reactiveCache.set(target, new WeakMap()); @@ -218,6 +209,7 @@ export function reactive( : basicProxyHandler(callback); const proxy = new Proxy(target, handler as ProxyHandler) as Reactive; reactivesForTarget.set(callback, proxy); + targets.set(proxy, target); } return reactivesForTarget.get(callback) as Reactive; } @@ -229,29 +221,29 @@ export function reactive( */ function basicProxyHandler(callback: Callback): ProxyHandler { return { - get(target: any, key: PropertyKey, proxy: Reactive) { - if (key === TARGET) { - return target; - } + get(target, key, receiver) { // non-writable non-configurable properties cannot be made reactive const desc = Object.getOwnPropertyDescriptor(target, key); if (desc && !desc.writable && !desc.configurable) { - return Reflect.get(target, key, proxy); + return Reflect.get(target, key, receiver); } observeTargetKey(target, key, callback); - return possiblyReactive(Reflect.get(target, key, proxy), callback); + return possiblyReactive(Reflect.get(target, key, receiver), callback); }, - set(target, key, value, proxy) { - const isNewKey = !objectHasOwnProperty.call(target, key); - const originalValue = Reflect.get(target, key, proxy); - const ret = Reflect.set(target, key, value, proxy); - if (isNewKey) { + set(target, key, value, receiver) { + const hadKey = objectHasOwnProperty.call(target, key); + const originalValue = Reflect.get(target, key, receiver); + const ret = Reflect.set(target, key, value, receiver); + if (!hadKey && objectHasOwnProperty.call(target, key)) { notifyReactives(target, KEYCHANGES); } // While Array length may trigger the set trap, it's not actually set by this // method but is updated behind the scenes, and the trap is not called with the // new value. We disable the "same-value-optimization" for it because of that. - if (originalValue !== value || (Array.isArray(target) && key === "length")) { + if ( + originalValue !== Reflect.get(target, key, receiver) || + (key === "length" && Array.isArray(target)) + ) { notifyReactives(target, key); } return ret; @@ -444,10 +436,8 @@ function collectionsProxyHandler( // property is read. const specialHandlers = rawTypeToFuncHandlers[targetRawType](target, callback); return Object.assign(basicProxyHandler(callback), { + // FIXME: probably broken when part of prototype chain since we ignore the receiver get(target: any, key: PropertyKey) { - if (key === TARGET) { - return target; - } if (objectHasOwnProperty.call(specialHandlers, key)) { return (specialHandlers as any)[key]; } diff --git a/tests/reactivity.test.ts b/tests/reactivity.test.ts index 751ee5a7..a02648fa 100644 --- a/tests/reactivity.test.ts +++ b/tests/reactivity.test.ts @@ -9,7 +9,7 @@ import { markRaw, toRaw, } from "../src"; -import { reactive, Reactive } from "../src/runtime/reactivity"; +import { reactive } from "../src/runtime/reactivity"; import { batched } from "../src/runtime/utils"; import { makeDeferred, @@ -1142,6 +1142,46 @@ describe("Reactivity", () => { expect(() => state.a).not.toThrow(); expect(state.a).toBe(obj.a); }); + + test("writing on object with reactive in prototype chain doesn't notify", async () => { + let n = 0; + const state = createReactive({ val: 0 }, () => n++); + const nonReactive = Object.create(state); + nonReactive.val++; + expect(n).toBe(0); + expect(toRaw(state)).toEqual({ val: 0 }); + expect(toRaw(nonReactive)).toEqual({ val: 1 }); + state.val++; + expect(n).toBe(1); + expect(toRaw(state)).toEqual({ val: 1 }); + expect(toRaw(nonReactive)).toEqual({ val: 1 }); + }); + + test("creating key on object with reactive in prototype chain doesn't notify", async () => { + let n = 0; + const parent = createReactive({}, () => n++); + const child = Object.create(parent); + Object.keys(parent); // Subscribe to key changes + child.val = 0; + expect(n).toBe(0); + }); + + test("reactive of object with reactive in prototype chain is not the object from the prototype chain", async () => { + const cb = () => {}; + const parent = createReactive({ val: 0 }, cb); + const child = createReactive(Object.create(parent), cb); + expect(child).not.toBe(parent); + }); + + test("can create reactive of object with non-reactive in prototype chain", async () => { + let n = 0; + const parent = markRaw({ val: 0 }); + const child = createReactive(Object.create(parent), () => n++); + child.val++; + expect(n).toBe(1); + expect(parent).toEqual({ val: 0 }); + expect(child).toEqual({ val: 1 }); + }); }); describe("Collections", () => { @@ -1699,12 +1739,12 @@ describe("toRaw", () => { const obj = { value: 1 }; const reactiveObj = reactive(obj); expect(reactiveObj).not.toBe(obj); - expect(toRaw(reactiveObj as Reactive)).toBe(obj); + expect(toRaw(reactiveObj)).toBe(obj); }); test("giving a non reactive to toRaw return the object itself", () => { const obj = { value: 1 }; - expect(toRaw(obj as Reactive)).toBe(obj); + expect(toRaw(obj)).toBe(obj); }); });