[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

@@ -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<ComponentNode, Callback>();
  *  relevant changes
  * @see reactive
  */
-export function useState<T extends object>(state: T): Reactive<T> | NonReactive<T> {
+export function useState<T extends object>(state: T): T {
   const node = getCurrent();
   let render = batchedRenderFunctions.get(node)!;
   if (!render) {
@@ -116,7 +109,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
     this.childEnv = env;
     for (const key in props) {
       const prop = props[key];
-      if (prop && typeof prop === "object" && prop[TARGET]) {
+      if (prop && typeof prop === "object" && targets.has(prop)) {
         props[key] = useState(prop);
       }
     }
@@ -240,7 +233,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
     currentNode = this;
     for (const key in props) {
       const prop = props[key];
-      if (prop && typeof prop === "object" && prop[TARGET]) {
+      if (prop && typeof prop === "object" && targets.has(prop)) {
         props[key] = useState(prop);
       }
     }

src/runtime/index.ts

@@ -12,7 +12,6 @@ import {
   comment,
 } from "./blockdom";
 import { mainEventHandler } from "./event_handling";
-export type { Reactive } from "./reactivity";
 
 config.shouldNormalizeDom = false;
 config.mainEventHandler = mainEventHandler;

src/runtime/reactivity.ts

@@ -1,25 +1,17 @@
 import { Callback } from "./utils";
 import { OwlError } from "./error_handling";
 
-// Allows to get the target of a Reactive (used for making a new Reactive from the underlying object)
-export const TARGET = Symbol("Target");
-// Escape hatch to prevent reactivity system to turn something into a reactive
-const SKIP = Symbol("Skip");
 // Special key to subscribe to, to be notified of key creation/deletion
 const KEYCHANGES = Symbol("Key changes");
 
+// The following types only exist to signify places where objects are expected
+// to be reactive or not, they provide no type checking benefit over "object"
 type Target = object;
+type Reactive<T extends Target> = T;
+
 type Collection = Set<any> | Map<any, any> | WeakMap<any, any>;
 type CollectionRawType = "Set" | "Map" | "WeakMap";
 
-export type Reactive<T extends Target = Target> = T & {
-  [TARGET]: any;
-};
-
-export type NonReactive<T extends Target = Target> = 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<Target>();
 /**
  * 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<T extends Target>(value: T): NonReactive<T> {
-  (value as any)[SKIP] = true;
-  return value as NonReactive<T>;
+export function markRaw<T extends Target>(value: T): T {
+  skipped.add(value);
+  return value;
 }
 
 /**
@@ -78,8 +71,8 @@ export function markRaw<T extends Target>(value: T): NonReactive<T> {
  * @param value a reactive value
  * @returns the underlying value
  */
-export function toRaw<T extends object>(value: Reactive<T>): T {
-  return value[TARGET] || value;
+export function toRaw<T extends Target, U extends Reactive<T>>(value: U | T): T {
+  return targets.has(value) ? (targets.get(value) as T) : value;
 }
 
 const targetToKeysToCallbacks = new WeakMap<Target, Map<PropertyKey, Set<Callback>>>();
@@ -164,8 +157,9 @@ export function getSubscriptions(callback: Callback) {
     };
   });
 }
-
-const reactiveCache = new WeakMap<Target, WeakMap<Callback, Reactive>>();
+// Maps reactive objects to the underlying target
+export const targets = new WeakMap<Reactive<Target>, Target>();
+const reactiveCache = new WeakMap<Target, WeakMap<Callback, Reactive<Target>>>();
 /**
  * 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<Target, WeakMap<Callback, Reactive>>();
  * 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<Target, WeakMap<Callback, Reactive>>();
  *  reactive has changed
  * @returns a proxy that tracks changes to it
  */
-export function reactive<T extends Target>(
-  target: T,
-  callback: Callback = () => {}
-): Reactive<T> | NonReactive<T> {
+export function reactive<T extends Target>(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<T>;
+  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<T extends Target>(
       : basicProxyHandler<T>(callback);
     const proxy = new Proxy(target, handler as ProxyHandler<T>) as Reactive<T>;
     reactivesForTarget.set(callback, proxy);
+    targets.set(proxy, target);
   }
   return reactivesForTarget.get(callback) as Reactive<T>;
 }
@@ -229,29 +221,29 @@ export function reactive<T extends Target>(
  */
 function basicProxyHandler<T extends Target>(callback: Callback): ProxyHandler<T> {
   return {
-    get(target: any, key: PropertyKey, proxy: Reactive<T>) {
-      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<T extends Collection>(
   // 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];
       }

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<typeof obj>)).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<typeof obj>)).toBe(obj);
+    expect(toRaw(obj)).toBe(obj);
   });
 });