Skip to content

Commit

Permalink
[FIX] reactivity: do not notify for NOOPs on collections
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sdegueldre authored and ged-odoo committed Sep 22, 2023
1 parent e7ebb92 commit 3937966
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/runtime/reactivity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
18 changes: 18 additions & 0 deletions tests/reactivity.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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", () => {
Expand All @@ -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 () => {
Expand Down

0 comments on commit 3937966

Please sign in to comment.