Skip to content

Commit

Permalink
Supports synchronous scheduling in effect.
Browse files Browse the repository at this point in the history
This broke for all signal changes after the first when using a synchronous `Scheduler` because `cancelNextCall = undefined` in the `.schedule` callback was getting invoked *before* the assignment to `cancelNextCall = scheduler.schedule(() => /* ... */)`, meaning the cancel function always exists. Since we used `cancelNextCall` to determine whether an operation was scheduled, `effect` incorrectly thought the action was still scheduled when it had actually already executed.

This is fixed by using a distinct `scheduled` boolean which looks a bit redundant, but is necessary for the synchronous `Scheduler` case. Added a regression test to be more clear about this requirement.
  • Loading branch information
dgp1130 committed Dec 2, 2024
1 parent 283fee9 commit 703b743
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 6 deletions.
29 changes: 29 additions & 0 deletions src/signals/effect.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Action, CancelAction, Scheduler } from 'hydroactive/signals.js';
import { effect } from './effect.js';
import { StabilityTracker } from './schedulers/stability-tracker.js';
import { TestScheduler } from './schedulers/test-scheduler.js';
Expand Down Expand Up @@ -139,6 +140,27 @@ describe('effect', () => {
dispose();
});

it('handles synchronous scheduling', () => {
const scheduler = new SyncScheduler();
const value = signal(1);
const action = jasmine.createSpy<() => void>('action')
.and.callFake(() => value());

const dispose = effect(action, scheduler);
expect(action).toHaveBeenCalledOnceWith();
action.calls.reset();

value.set(2);
expect(action).toHaveBeenCalledOnceWith();
action.calls.reset();

// Regression test: Dropped second+ post-init schedule operations.
value.set(3);
expect(action).toHaveBeenCalledOnceWith();

dispose();
});

describe('dispose', () => {
it('cleans up the effect', () => {
const tracker = StabilityTracker.from();
Expand Down Expand Up @@ -197,3 +219,10 @@ describe('effect', () => {
});
});
});

class SyncScheduler implements Scheduler {
public schedule(action: Action): CancelAction {
action();
return () => {};
}
}
14 changes: 8 additions & 6 deletions src/signals/effect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,24 @@ export function effect(
});

let cancelNextCall: CancelAction | undefined;
let scheduled = false;
consumer.listen(() => {
// If already scheduled, nothing to do.
if (cancelNextCall) return;
// It might look like we could drop `scheduled` and use the presence of
// `cancelNextCall` to know whether an event is scheduled, but this would
// not work for a synchronous `Scheduler`.
if (scheduled) return;

scheduled = true;
cancelNextCall = scheduler.schedule(() => {
cancelNextCall = undefined;
scheduled = false;
consumer.record(callback);
});
});

return () => {
cancelInitialCall();
if (cancelNextCall) {
cancelNextCall();
cancelNextCall = undefined;
}
if (scheduled) cancelNextCall!();
consumer.destroy();
};
}
Expand Down

0 comments on commit 703b743

Please sign in to comment.