Skip to content

Commit bf1c12c

Browse files
alxhubalxhub
committed
fix(forms): memoize reads of child fields in signal forms (#65802)
Previously, navigating a `FieldTree` in signal forms involved reactive reads of the value of the parent field(s), both directly and via `.childrenMap()`. This meant that on _any_ change to the value of a field, reactive notifications would trigger updates of computeds, reruns of effects, etc. So for example, this effect would run on every change to the form: ```ts const f = form(signal({data: 'abc', unrelated: 0})); effect(() => { // accessing f.data incurs a dependency on f().value() which changes // on every change in the whole form console.log(f.data().value()); }); ``` This is deeply counterintuitive and troublesome when attempting to write effect logic, and also results in `computed`s unnecessarily updating. This change introduces the concept of a "reader" computed, which memoizes the access of a field at a given key via the reactive graph. With this, the same `f.data` access above now depends on the `data` reader in `f` only, which is effectively a constant computed. As a result, the effect only reruns on changes to `data`'s value, as intended. PR Close #65802
1 parent 2c12661 commit bf1c12c

File tree

7 files changed

+370
-163
lines changed

7 files changed

+370
-163
lines changed

‎packages/forms/signals/compat/src/compat_structure.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,12 +107,14 @@ export class CompatStructure extends FieldNodeStructure {
107107
override root: FieldNode;
108108
override pathKeys: Signal<readonly string[]>;
109109
override readonly children = signal([]);
110-
override readonly childrenMap = signal(undefined);
110+
override readonly childrenMap = computed(() => undefined);
111111
override readonly parent: ParentFieldNode | undefined;
112112
override readonly fieldManager: FormFieldManager;
113113

114114
constructor(node: FieldNode, options: CompatFieldNodeOptions) {
115-
super(options.logic);
115+
super(options.logic, node, () => {
116+
throw new Error(`Compat nodes don't have children.`);
117+
});
116118
this.value = getControlValueSignal(options);
117119
this.parent = getParentFromOptions(options);
118120
this.root = this.parent?.structure.root ?? node;

‎packages/forms/signals/src/field/proxy.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,16 @@ export const FIELD_PROXY_HANDLER: ProxyHandler<() => FieldNode> = {
4141
// Allow access to the iterator. This allows the user to spread the field array into a
4242
// standard array in order to call methods like `filter`, `map`, etc.
4343
if (p === Symbol.iterator) {
44-
return Array.prototype[p as any];
44+
return () => {
45+
// When creating an iterator, we need to account for reactivity. The iterator itself will
46+
// read things each time `.next()` is called, but that may happen outside of the context
47+
// where the iterator was created (e.g. with `@for`, actual diffing happens outside the
48+
// reactive context of the template).
49+
//
50+
// Instead, side-effectfully read the value here to ensure iterator creation is reactive.
51+
tgt.value();
52+
return Array.prototype[Symbol.iterator].apply(tgt.fieldProxy);
53+
};
4554
}
4655
// Note: We can consider supporting additional array methods if we want in the future,
4756
// but they should be thoroughly tested. Just forwarding the method directly from the

‎packages/forms/signals/src/field/state.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import type {Field} from '../api/field_directive';
1111
import type {Debouncer, DisabledReason} from '../api/types';
1212
import {DEBOUNCER} from './debounce';
1313
import type {FieldNode} from './node';
14-
import {reduceChildren, shortCircuitTrue} from './util';
14+
import {shortCircuitTrue} from './util';
1515

1616
/**
1717
* The non-validation and non-submit state associated with a `FieldNode`, such as touched and dirty
@@ -76,8 +76,7 @@ export class FieldNodeState {
7676
*/
7777
readonly dirty: Signal<boolean> = computed(() => {
7878
const selfDirtyValue = this.selfDirty() && !this.isNonInteractive();
79-
return reduceChildren(
80-
this.node,
79+
return this.node.structure.reduceChildren(
8180
selfDirtyValue,
8281
(child, value) => value || child.nodeState.dirty(),
8382
shortCircuitTrue,
@@ -93,8 +92,7 @@ export class FieldNodeState {
9392
*/
9493
readonly touched: Signal<boolean> = computed(() => {
9594
const selfTouchedValue = this.selfTouched() && !this.isNonInteractive();
96-
return reduceChildren(
97-
this.node,
95+
return this.node.structure.reduceChildren(
9896
selfTouchedValue,
9997
(child, value) => value || child.nodeState.touched(),
10098
shortCircuitTrue,

0 commit comments

Comments
 (0)