From 3937966b743e6d95dd83aa8f9ee32aaa597d45c4 Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Fri, 22 Sep 2023 14:13:07 +0200
Subject: [PATCH] [FIX] reactivity: do not notify for NOOPs on collections

Previously, if a reactive was observing the presence of an item in a
set that was originally not present, it would get notified when that
item was "deleted" from the set (even though there was nothing to delete
and the set did not change). The same applied to the key already being
present and then being "added". The same thing occured with Map, both
for presence but also for values (setting a key to the value it was
already set to would notify).

This commit fixes that.
---
 src/runtime/reactivity.ts |  2 +-
 tests/reactivity.test.ts  | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/runtime/reactivity.ts b/src/runtime/reactivity.ts
index 210e38ec..43ae74b9 100644
--- a/src/runtime/reactivity.ts
+++ b/src/runtime/reactivity.ts
@@ -368,7 +368,7 @@ function delegateAndNotify(
     if (hadKey !== hasKey) {
       notifyReactives(target, KEYCHANGES);
     }
-    if (originalValue !== value) {
+    if (originalValue !== target[getterName](key)) {
       notifyReactives(target, key);
     }
     return ret;
diff --git a/tests/reactivity.test.ts b/tests/reactivity.test.ts
index 9d67f1d5..ed6ef775 100644
--- a/tests/reactivity.test.ts
+++ b/tests/reactivity.test.ts
@@ -1292,6 +1292,12 @@ describe("Collections", () => {
 
       state.add(3); // setting unobserved key doesn't notify
       expect(observer).toHaveBeenCalledTimes(3);
+      expect(state.has(3)).toBe(true); // subscribe to 3
+      state.add(3); // adding observed key doesn't notify if key was already present
+      expect(observer).toHaveBeenCalledTimes(3);
+      expect(state.has(4)).toBe(false); // subscribe to 4
+      state.delete(4); // deleting observed key doesn't notify if key was already not present
+      expect(observer).toHaveBeenCalledTimes(3);
     });
 
     test("iterating on keys returns reactives", async () => {
@@ -1485,6 +1491,12 @@ describe("Collections", () => {
 
       state.set(3, 4); // setting unobserved key doesn't notify
       expect(observer).toHaveBeenCalledTimes(3);
+      expect(state.has(3)).toBe(true); // subscribe to 3
+      state.set(3, 4); // setting the same value doesn't notify
+      expect(observer).toHaveBeenCalledTimes(3);
+      expect(state.has(4)).toBe(false); // subscribe to 4
+      state.delete(4); // deleting observed key doesn't notify if key was already not present
+      expect(observer).toHaveBeenCalledTimes(3);
     });
 
     test("checking for a key with 'get' subscribes the callback to changes to that key", () => {
@@ -1510,6 +1522,12 @@ describe("Collections", () => {
 
       state.set(3, 4); // setting unobserved key doesn't notify
       expect(observer).toHaveBeenCalledTimes(3);
+      expect(state.get(3)).toBe(4); // subscribe to 3
+      state.set(3, 4); // setting the same value doesn't notify
+      expect(observer).toHaveBeenCalledTimes(3);
+      expect(state.get(4)).toBe(undefined); // subscribe to 4
+      state.delete(4); // deleting observed key doesn't notify if key was already not present
+      expect(observer).toHaveBeenCalledTimes(3);
     });
 
     test("getting values returns a reactive", async () => {