Skip to content

Commit

Permalink
fix(core): treat exceptions in equal as part of computation
Browse files Browse the repository at this point in the history
Prevent leaking signal reads and exceptions from a custom `equal`
function of a producer `computed()` to a consumer.

Upstream tc39/proposal-signals#90 with a notable
change: Angular does **not** track reactive reads in custom `equal`
implementations.
  • Loading branch information
leonsenft committed Nov 27, 2024
1 parent 0f1c718 commit da8e03e
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 6 deletions.
17 changes: 11 additions & 6 deletions packages/core/primitives/signals/src/computed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
producerUpdateValueVersion,
REACTIVE_NODE,
ReactiveNode,
setActiveConsumer,
SIGNAL,
} from './graph';

Expand Down Expand Up @@ -119,21 +120,25 @@ const COMPUTED_NODE = /* @__PURE__ */ (() => {

const prevConsumer = consumerBeforeComputation(node);
let newValue: unknown;
let wasEqual = false;
try {
newValue = node.computation();
// We want to mark this node as errored if calling `equal` throws; however, we don't want
// to track any reactive reads inside `equal`.
setActiveConsumer(null);
wasEqual =
oldValue !== UNSET &&
oldValue !== ERRORED &&
newValue !== ERRORED &&
node.equal(oldValue, newValue);
} catch (err) {
newValue = ERRORED;
node.error = err;
} finally {
consumerAfterComputation(node, prevConsumer);
}

if (
oldValue !== UNSET &&
oldValue !== ERRORED &&
newValue !== ERRORED &&
node.equal(oldValue, newValue)
) {
if (wasEqual) {
// No change to `valueVersion` - old and new values are
// semantically equivalent.
node.value = oldValue;
Expand Down
90 changes: 90 additions & 0 deletions packages/core/test/signals/computed_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,4 +201,94 @@ describe('computed', () => {
] as ReactiveNode;
expect(node.debugName).toBe('computedSignal');
});

describe('with custom equal', () => {
it('should cache exceptions thrown by equal', () => {
const s = signal(0);

let computedRunCount = 0;
let equalRunCount = 0;
const c = computed(
() => {
computedRunCount++;
return s();
},
{
equal: () => {
equalRunCount++;
throw new Error('equal');
},
},
);

// equal() isn't run for the initial computation.
expect(c()).toBe(0);
expect(computedRunCount).toBe(1);
expect(equalRunCount).toBe(0);

s.set(1);

// Error is thrown by equal().
expect(() => c()).toThrowError('equal');
expect(computedRunCount).toBe(2);
expect(equalRunCount).toBe(1);

// Error is cached; c throws again without needing to rerun computation or equal().
expect(() => c()).toThrowError('equal');
expect(computedRunCount).toBe(2);
expect(equalRunCount).toBe(1);
});

it('should not track signal reads inside equal', () => {
const value = signal(1);
const epsilon = signal(0.5);

let innerRunCount = 0;
let equalRunCount = 0;
const inner = computed(
() => {
innerRunCount++;
return value();
},
{
equal: (a, b) => {
equalRunCount++;
return Math.abs(a - b) < epsilon();
},
},
);

let outerRunCount = 0;
const outer = computed(() => {
outerRunCount++;
return inner();
});

// Everything runs the first time.
expect(outer()).toBe(1);
expect(innerRunCount).toBe(1);
expect(outerRunCount).toBe(1);

// Difference is less than epsilon().
value.set(1.2);

// `inner` reruns because `value` was changed, and `equal` is called for the first time.
// `outer does not rerun because `equal` determined that `inner` had not changed.
expect(outer()).toBe(1);
expect(innerRunCount).toBe(2);
expect(outerRunCount).toBe(1);
expect(equalRunCount).toBe(1);

// Previous difference is now greater than epsilon().
epsilon.set(0.1);

// While changing `epsilon` would change the outcome of the `inner`, we don't rerun it
// because we intentionally don't track reactive reads in `equal`. Equally important is that
// the signal read in `equal` doesn't leak into the outer reactive context either.
expect(outer()).toBe(1);
expect(innerRunCount).toBe(2);
expect(outerRunCount).toBe(1);
expect(equalRunCount).toBe(1);
});
});
});

0 comments on commit da8e03e

Please sign in to comment.