Skip to content

Commit

Permalink
[FIX] utils: fix calls to batched callback from within the callback
Browse files Browse the repository at this point in the history
Previously, calling the batched function from within the callback being
batched would fail as it would be treated as part of the same batch.
This commit fixes that by scheduling the reset of the "called" flag
before calling the callback. This means that all microtasks that were
already in the microtask queue when a batch is about to run are treated
as part of the batch, and all microtasks that will be added by the
callback are not.
  • Loading branch information
sdegueldre authored and ged-odoo committed Mar 29, 2022
1 parent 7611ea6 commit d80fad7
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 4 deletions.
10 changes: 6 additions & 4 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@ export function batched(callback: Callback): Callback {
await Promise.resolve();
if (!called) {
called = true;
callback();
// wait for all calls in this microtick to fall through before resetting "called"
// so that only the first call to the batched function calls the original callback
await Promise.resolve();
called = false;
// so that only the first call to the batched function calls the original callback.
// Schedule this before calling the callback so that calls to the batched function
// within the callback will proceed only after resetting called to false, and have
// a chance to execute the callback again
Promise.resolve().then(() => (called = false));
callback();
}
};
}
Expand Down
40 changes: 40 additions & 0 deletions tests/reactivity.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,46 @@ describe("Reactivity", () => {
expect(n).toBe(1); // two operations but only one notification
});

test("batched: modifying the reactive in the callback doesn't break reactivity", async () => {
let n = 0;
let obj = { a: 1 };
const state = createReactive(
obj,
batched(() => {
state.a; // subscribe to a
state.a = 2;
n++;
})
);
expect(n).toBe(0);
state.a = 2;
expect(n).toBe(0);
await nextMicroTick();
expect(n).toBe(0); // key has not be read yet
state.a = state.a + 5; // key is read and then modified
expect(n).toBe(0);
await nextMicroTick();
expect(n).toBe(1);
// the write a = 2 inside the batched callback triggered another notification, wait for it
await nextMicroTick();
expect(n).toBe(2);
// Should now be stable as we're writing the same value again
await nextMicroTick();
expect(n).toBe(2);

// Do it again to check it's not broken
state.a = state.a + 5; // key is read and then modified
expect(n).toBe(2);
await nextMicroTick();
expect(n).toBe(3);
// the write a = 2 inside the batched callback triggered another notification, wait for it
await nextMicroTick();
expect(n).toBe(4);
// Should now be stable as we're writing the same value again
await nextMicroTick();
expect(n).toBe(4);
});

test("setting property to same value does not trigger callback", async () => {
let n = 0;
const state = createReactive({ a: 1 }, () => n++);
Expand Down
20 changes: 20 additions & 0 deletions tests/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,24 @@ describe("batched", () => {
await nextMicroTick();
expect(n).toBe(1);
});

test("calling batched function from within the callback is not treated as part of the original batch", async () => {
let n = 0;
let fn = batched(() => {
n++;
if (n === 1) {
fn();
}
});

expect(n).toBe(0);
fn();
expect(n).toBe(0);
await nextMicroTick(); // First batch
expect(n).toBe(1);
await nextMicroTick(); // Second batch initiated from within the callback
expect(n).toBe(2);
await nextMicroTick();
expect(n).toBe(2);
});
});

0 comments on commit d80fad7

Please sign in to comment.