fix(core): treat reads/exceptions in equal as part of computation#55818
fix(core): treat reads/exceptions in equal as part of computation#55818leonsenft wants to merge 2 commits intoangular:mainfrom
equal as part of computation#55818Conversation
equal as part of computationequal as part of computation
|
@alxhub PTAL–I don't currently have permission to assign you as a reviewer. |
pkozlowski-opensource
left a comment
There was a problem hiding this comment.
Discussed in person, here is the summary.
To start with, there are probably 2 related concerns addressed here:
- exceptions thrown in the
equalsfunction body - reactive context of the
equalsfunction body
When it comes to exceptions, it is reasonable to assume that an error thrown in the equals function body marks the reactive node as "errored". The reasoning here is that is a user code provided during a signal construction. In this sense it can be considered an integral part of a reactive node.
As for reactive tracking inside equals: the outcome of the discussion is that we should not track signal reads there. The reasoning being that equals should be a pure function over it arguments. Please note that this is different from the current state of the TC39 proposal (see tc39/proposal-signals#90) so we will need to allign.
The code in this PR need to change to accommodate the above.
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.
|
@pkozlowski-opensource I've updated this change to no longer track signal reads in |
| expect(() => c()).toThrowError('equal'); | ||
| expect(computedRunCount).toBe(2); | ||
| expect(equalRunCount).toBe(1); | ||
| }); |
There was a problem hiding this comment.
I would add another case here that marks a computed as dirty and have a conditional throw in equals - just to make sure that we can get out of the errored state
There was a problem hiding this comment.
This actually exposes an interesting problem with not tracking reactive reads in equal.
let shouldThrow = false;
const source = signal(0);
const derived = computed(source, {
equal: (a, b) => {
if (shouldThrow) {
throw new Error('equal');
}
return defaultEquals(a, b);
},
});
expect(derived()).toBe(0);
// Begin throwing from equal.
shouldThrow = true;
source.set(1);
expect(() => derived()).toThrowError('equal');
// Stop throwing from equal.
shouldThrow = false;
expect(derived()).toBe(1); // ERROR: still throws cached error.Even if equal stops throwing, because we cached the result, the computed won't stop throwing until the source signal changes again.
source.set(2);
expect(derived()).toBe(2); // Now this stops throwing.Is this desirable? One could make the argument that if we did track reactivity in equal, then the shouldThrow should be a signal, and this would all work as expected.
There was a problem hiding this comment.
Discussed with @alxhub and came to the conclusion that this behavior is by design, and that the computed should cache the error until the source signal changes again (since we're assuming that equals is pure). I've updated the test case accordingly.
ENAML
left a comment
There was a problem hiding this comment.
Reviewed-for: primitives-shared
|
Started a TGP on this one. |
|
Green TGP after deflaking |
|
TESTED=Green TGP after deflaking |
|
This PR was merged into the repository by commit f8b8e80. The changes were merged into the following branches: main, 19.1.x |
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. PR Close #55818
…r#55818) 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. PR Close angular#55818
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Prevent leaking signal reads and exceptions from a custom
equalfunction of a producercomputed()to a consumer.Upstream tc39/proposal-signals#90.