fix(forms): memoize reads of child fields in signal forms #65802

alxhub · 2025-12-02T23:48:37Z

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:

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 computeds 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.

Previously, several values were being passed into the creation of `FieldNodeStructure`s that were only used in the creation of child nodes. Separately, we also passed a `createChildNode` function which these values were passed back into. Instead, this moves the small bit of logic from structure.ts behind the `createChildNode` callback, which reduces the passing of values back-and- forth and gives `createChildNode` a much more suitable signature.

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

packages/forms/signals/src/field/structure.ts

leonsenft · 2025-12-03T00:40:29Z

packages/forms/signals/src/field/structure.ts

-    /** The logic to apply to this field. */
-    readonly logic: LogicNode,
-  ) {}
+  constructor(logic: LogicNode, node: FieldNode, createChildNode: ChildNodeCtor) {


nit: Use parameter properties?

Suggested change

constructor(logic: LogicNode, node: FieldNode, createChildNode: ChildNodeCtor) {

constructor(

readonly logic: LogicNode,

readonly node: FieldNode,

readonly createChildNode: ChildNodeCtor,

) {}

I'm starting to move away from non-erasable syntax in the TS I write.

I wonder if we should have a lint for it, I didn't realize I shouldn't be doing this

It's more just something I'm experimenting with, it's not part of our style guide (yet).

Is the idea that eventually we'll be able to execute the TS source as JS without any compilation for faster dev if we exclusively use non-erasable syntax?

packages/forms/signals/src/field/structure.ts

leonsenft · 2025-12-03T01:00:45Z

packages/forms/signals/src/field/structure.ts

+        // Remove fields that have disappeared since the last time this map was computed.
+        if (prevData !== undefined) {


This seems like a good candidate to move to its own function to improve readability.

Maybe - it depends on type narrowing from the parent function and modifies local variable state though, so it'd be tricky to extract like this.

What parent function? It only appears to modify data, which isn't initialized before this block. Couldn't you simply do something like:

if (parentIsArray) { data = remainingDataFromArray(prevData, value); } else { data = remainingDataFromObject(prevData, value); }

packages/forms/signals/src/field/structure.ts

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.

mmalerba · 2025-12-03T16:22:52Z

packages/forms/signals/src/field/node.ts

+    let childLogic: LogicNode;
+    if (isArray) {
+      // Fields for array elements have their logic defined by the `element` mechanism.
+      // TODO: other dynamic data


Is this TODO in reference to dynamic object logic? if so I think it can go away, as we have that now

I'm not sure - it was moved from the other file, I didn't write it (just now, anyway)

mmalerba · 2025-12-03T16:28:06Z

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

-        return Array.prototype[p as any];
+        return () => {
+          tgt.value();
+          return Array.prototype[Symbol.iterator].apply(tgt.fieldProxy);


Do you actually need the .apply? I would think since its accessed as a property of the proxy, the this should be correct (unless the () => messes that up or something)

Yep, the () messes it up - this in the arrow function is the proxy handler object, not the proxy itself. I think this is true even without the arrow function - we need the this to be the proxy.

mmalerba · 2025-12-03T16:31:20Z

packages/forms/signals/src/field/structure.ts

-    /** The logic to apply to this field. */
-    readonly logic: LogicNode,
-  ) {}
+  constructor(logic: LogicNode, node: FieldNode, createChildNode: ChildNodeCtor) {


I wonder if we should have a lint for it, I didn't realize I shouldn't be doing this

leonsenft

Please use a fixup / additional commit in the future as the amended commit prevents GitHub from showing me what's changed since my last review. Fortunately this change is small enough that manually cross-referencing my old comments and the new code was feasible.

leonsenft · 2025-12-03T18:26:19Z

packages/forms/signals/src/field/structure.ts

-    /** The logic to apply to this field. */
-    readonly logic: LogicNode,
-  ) {}
+  constructor(logic: LogicNode, node: FieldNode, createChildNode: ChildNodeCtor) {


Is the idea that eventually we'll be able to execute the TS source as JS without any compilation for faster dev if we exclusively use non-erasable syntax?

leonsenft · 2025-12-03T18:40:24Z

packages/forms/signals/src/field/structure.ts

+        // Remove fields that have disappeared since the last time this map was computed.
+        if (prevData !== undefined) {


What parent function? It only appears to modify data, which isn't initialized before this block. Couldn't you simply do something like:

if (parentIsArray) { data = remainingDataFromArray(prevData, value); } else { data = remainingDataFromObject(prevData, value); }

alxhub · 2025-12-03T20:52:47Z

This PR was merged into the repository. The changes were merged into the following branches:

main: b96f65a
21.0.x: bf1c12c

Previously, several values were being passed into the creation of `FieldNodeStructure`s that were only used in the creation of child nodes. Separately, we also passed a `createChildNode` function which these values were passed back into. Instead, this moves the small bit of logic from structure.ts behind the `createChildNode` callback, which reduces the passing of values back-and- forth and gives `createChildNode` a much more suitable signature. PR Close #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

angular-robot bot added the area: forms label Dec 2, 2025

ngbot bot added this to the Backlog milestone Dec 2, 2025

alxhub force-pushed the sf/child-readers branch from c7e0f8d to 83d7050 Compare December 2, 2025 23:53

alxhub force-pushed the sf/child-readers branch from 83d7050 to 2f79277 Compare December 2, 2025 23:56

leonsenft self-requested a review December 3, 2025 00:17

leonsenft requested changes Dec 3, 2025

View reviewed changes

alxhub force-pushed the sf/child-readers branch from 2f79277 to e67abd4 Compare December 3, 2025 16:29

alxhub added the target: patch This PR is targeted for the next patch release label Dec 3, 2025

mmalerba reviewed Dec 3, 2025

View reviewed changes

leonsenft approved these changes Dec 3, 2025

View reviewed changes

fixup! fix(forms): memoize reads of child fields in signal forms

93ac2c3

alxhub added the action: merge The PR is ready for merge by the caretaker label Dec 3, 2025

alxhub closed this in 37472e9 Dec 3, 2025

This was referenced Dec 3, 2025

signal-forms: Field value change signals update frequently #63627

Closed

Signal Forms: Infinite loop when setting a field value in an effect #65472

Closed

JeanMeche mentioned this pull request Dec 7, 2025

Repeated value changed signal firing with the same value #65904

Closed

		// Remove fields that have disappeared since the last time this map was computed.
		if (prevData !== undefined) {

fix(forms): memoize reads of child fields in signal forms #65802

fix(forms): memoize reads of child fields in signal forms #65802

Uh oh!

Conversation

alxhub commented Dec 2, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonsenft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alxhub Dec 3, 2025 •

edited

Loading