Skip to content

Commit 6d74755

Browse files
kirjsalxhub
kirjs
authored and
alxhub
committed
fix(forms): Reuse key in parent in compat structure
This fixes the "Compat nodes do not use keyInParent." error when trying to bind compatField. (cherry picked from commit 179b4cb)
1 parent bb0061b commit 6d74755

File tree

3 files changed

+196
-60
lines changed

3 files changed

+196
-60
lines changed

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,7 @@ function getControlValueSignal<T>(options: CompatFieldNodeOptions) {
101101
*/
102102
export class CompatStructure extends FieldNodeStructure {
103103
override value: WritableSignal<unknown>;
104-
override keyInParent: Signal<string> = (() => {
105-
throw new Error('Compat nodes do not use keyInParent.');
106-
}) as unknown as Signal<string>;
104+
override keyInParent: Signal<string>;
107105
override root: FieldNode;
108106
override pathKeys: Signal<readonly string[]>;
109107
override readonly children = signal([]);
@@ -119,6 +117,11 @@ export class CompatStructure extends FieldNodeStructure {
119117
this.parent = getParentFromOptions(options);
120118
this.root = this.parent?.structure.root ?? node;
121119
this.fieldManager = getFieldManagerFromOptions(options);
120+
121+
const identityInParent = options.kind === 'child' ? options.identityInParent : undefined;
122+
const initialKeyInParent = options.kind === 'child' ? options.initialKeyInParent : undefined;
123+
this.keyInParent = this.createKeyInParent(options, identityInParent, initialKeyInParent);
124+
122125
this.pathKeys = computed(() =>
123126
this.parent ? [...this.parent.structure.pathKeys(), this.keyInParent()] : [],
124127
);

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

Lines changed: 93 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,85 @@ export abstract class FieldNodeStructure {
161161
this.injector.destroy();
162162
}
163163

164+
/**
165+
* Creates a keyInParent signal for a field node.
166+
*
167+
* For root nodes, returns ROOT_KEY_IN_PARENT which throws when accessed.
168+
* For child nodes, creates a computed that tracks the field's current key in its parent,
169+
* with special handling for tracked array elements.
170+
*
171+
* @param options The field node options
172+
* @param identityInParent The tracking identity (only for tracked array children)
173+
* @param initialKeyInParent The initial key in parent (only for child nodes)
174+
* @returns A signal representing the field's key in its parent
175+
*/
176+
protected createKeyInParent(
177+
options: FieldNodeOptions,
178+
identityInParent: TrackingKey | undefined,
179+
initialKeyInParent: string | undefined,
180+
): Signal<string> {
181+
if (options.kind === 'root') {
182+
return ROOT_KEY_IN_PARENT;
183+
}
184+
185+
if (identityInParent === undefined) {
186+
const key = initialKeyInParent!;
187+
return computed(() => {
188+
if (this.parent!.structure.getChild(key) !== this.node) {
189+
throw new Error(
190+
`RuntimeError: orphan field, looking for property '${key}' of ${getDebugName(this.parent!)}`,
191+
);
192+
}
193+
return key;
194+
});
195+
} else {
196+
let lastKnownKey = initialKeyInParent!;
197+
return computed(() => {
198+
// TODO(alxhub): future perf optimization: here we depend on the parent's value, but most
199+
// changes to the value aren't structural - they aren't moving around objects and thus
200+
// shouldn't affect `keyInParent`. We currently mitigate this issue via `lastKnownKey`
201+
// which avoids a search.
202+
const parentValue = this.parent!.structure.value();
203+
if (!isArray(parentValue)) {
204+
// It should not be possible to encounter this error. It would require the parent to
205+
// change from an array field to non-array field. However, in the current implementation
206+
// a field's parent can never change.
207+
throw new Error(
208+
`RuntimeError: orphan field, expected ${getDebugName(this.parent!)} to be an array`,
209+
);
210+
}
211+
212+
// Check the parent value at the last known key to avoid a scan.
213+
// Note: lastKnownKey is a string, but we pretend to typescript like its a number,
214+
// since accessing someArray['1'] is the same as accessing someArray[1]
215+
const data = parentValue[lastKnownKey as unknown as number];
216+
if (
217+
isObject(data) &&
218+
data.hasOwnProperty(this.parent!.structure.identitySymbol) &&
219+
data[this.parent!.structure.identitySymbol] === identityInParent
220+
) {
221+
return lastKnownKey;
222+
}
223+
224+
// Otherwise, we need to check all the keys in the parent.
225+
for (let i = 0; i < parentValue.length; i++) {
226+
const data = parentValue[i];
227+
if (
228+
isObject(data) &&
229+
data.hasOwnProperty(this.parent!.structure.identitySymbol) &&
230+
data[this.parent!.structure.identitySymbol] === identityInParent
231+
) {
232+
return (lastKnownKey = i.toString());
233+
}
234+
}
235+
236+
throw new Error(
237+
`RuntimeError: orphan field, can't find element in array ${getDebugName(this.parent!)}`,
238+
);
239+
});
240+
}
241+
}
242+
164243
protected createChildrenMap(): Signal<ChildrenData | undefined> {
165244
return linkedSignal({
166245
source: this.value,
@@ -363,64 +442,21 @@ export class ChildFieldNodeStructure extends FieldNodeStructure {
363442

364443
this.root = this.parent.structure.root;
365444

366-
this.pathKeys = computed(() => [...parent.structure.pathKeys(), this.keyInParent()]);
367-
368-
if (identityInParent === undefined) {
369-
const key = initialKeyInParent;
370-
this.keyInParent = computed(() => {
371-
if (parent.structure.getChild(key) !== node) {
372-
throw new Error(
373-
`RuntimeError: orphan field, looking for property '${key}' of ${getDebugName(parent)}`,
374-
);
375-
}
376-
return key;
377-
});
378-
} else {
379-
let lastKnownKey = initialKeyInParent;
380-
this.keyInParent = computed(() => {
381-
// TODO(alxhub): future perf optimization: here we depend on the parent's value, but most
382-
// changes to the value aren't structural - they aren't moving around objects and thus
383-
// shouldn't affect `keyInParent`. We currently mitigate this issue via `lastKnownKey`
384-
// which avoids a search.
385-
const parentValue = parent.structure.value();
386-
if (!isArray(parentValue)) {
387-
// It should not be possible to encounter this error. It would require the parent to
388-
// change from an array field to non-array field. However, in the current implementation
389-
// a field's parent can never change.
390-
throw new Error(
391-
`RuntimeError: orphan field, expected ${getDebugName(parent)} to be an array`,
392-
);
393-
}
394-
395-
// Check the parent value at the last known key to avoid a scan.
396-
// Note: lastKnownKey is a string, but we pretend to typescript like its a number,
397-
// since accessing someArray['1'] is the same as accessing someArray[1]
398-
const data = parentValue[lastKnownKey as unknown as number];
399-
if (
400-
isObject(data) &&
401-
data.hasOwnProperty(parent.structure.identitySymbol) &&
402-
data[parent.structure.identitySymbol] === identityInParent
403-
) {
404-
return lastKnownKey;
405-
}
406-
407-
// Otherwise, we need to check all the keys in the parent.
408-
for (let i = 0; i < parentValue.length; i++) {
409-
const data = parentValue[i];
410-
if (
411-
isObject(data) &&
412-
data.hasOwnProperty(parent.structure.identitySymbol) &&
413-
data[parent.structure.identitySymbol] === identityInParent
414-
) {
415-
return (lastKnownKey = i.toString());
416-
}
417-
}
445+
this.keyInParent = this.createKeyInParent(
446+
{
447+
kind: 'child',
448+
parent,
449+
pathNode: undefined!,
450+
logic,
451+
initialKeyInParent,
452+
identityInParent,
453+
fieldAdapter: undefined!,
454+
},
455+
identityInParent,
456+
initialKeyInParent,
457+
);
418458

419-
throw new Error(
420-
`RuntimeError: orphan field, can't find element in array ${getDebugName(parent)}`,
421-
);
422-
});
423-
}
459+
this.pathKeys = computed(() => [...parent.structure.pathKeys(), this.keyInParent()]);
424460

425461
this.value = deepSignal(this.parent.structure.value, this.keyInParent);
426462
this.childrenMap = this.createChildrenMap();

‎packages/forms/signals/test/web/compat_form.spec.ts‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.dev/license
7+
*/
8+
9+
import {Component, provideZonelessChangeDetection, signal} from '@angular/core';
10+
import {FormControl} from '@angular/forms';
11+
import {TestBed} from '@angular/core/testing';
12+
import {Field} from '../../public_api';
13+
import {compatForm} from '../../compat';
14+
15+
describe('compatForm with [field] directive', () => {
16+
beforeEach(() => {
17+
TestBed.configureTestingModule({
18+
providers: [provideZonelessChangeDetection()],
19+
});
20+
});
21+
22+
it('should bind compat form to input with [field] directive', () => {
23+
@Component({
24+
imports: [Field],
25+
template: `
26+
<input [field]="f.name" />
27+
<input type="number" [field]="f.age" />
28+
`,
29+
})
30+
class TestCmp {
31+
readonly cat = signal({
32+
name: new FormControl('pirojok-the-cat', {nonNullable: true}),
33+
age: new FormControl(5, {nonNullable: true}),
34+
});
35+
readonly f = compatForm(this.cat);
36+
}
37+
38+
const fixture = act(() => TestBed.createComponent(TestCmp));
39+
const inputs = fixture.nativeElement.querySelectorAll('input');
40+
const nameInput = inputs[0] as HTMLInputElement;
41+
const ageInput = inputs[1] as HTMLInputElement;
42+
43+
expect(nameInput.value).toBe('pirojok-the-cat');
44+
expect(ageInput.value).toBe('5');
45+
});
46+
47+
it('should bind root-level FormControl to input with [field] directive', () => {
48+
@Component({
49+
imports: [Field],
50+
template: `<input [field]="f" />`,
51+
})
52+
class TestCmp {
53+
readonly cat = signal(new FormControl('pirojok-the-cat', {nonNullable: true}));
54+
readonly f = compatForm(this.cat);
55+
}
56+
57+
const fixture = act(() => TestBed.createComponent(TestCmp));
58+
const input = fixture.nativeElement.querySelector('input') as HTMLInputElement;
59+
60+
expect(input.value).toBe('pirojok-the-cat');
61+
});
62+
63+
it('should bind fields when FormControls are mixed with regular values', () => {
64+
@Component({
65+
imports: [Field],
66+
template: `
67+
<input [field]="f.name" />
68+
<input type="number" [field]="f.age" />
69+
`,
70+
})
71+
class TestCmp {
72+
readonly cat = signal({
73+
name: new FormControl('fluffy', {nonNullable: true}),
74+
age: 3,
75+
species: 'cat',
76+
});
77+
readonly f = compatForm(this.cat);
78+
}
79+
80+
const fixture = act(() => TestBed.createComponent(TestCmp));
81+
const inputs = fixture.nativeElement.querySelectorAll('input');
82+
const nameInput = inputs[0] as HTMLInputElement;
83+
const ageInput = inputs[1] as HTMLInputElement;
84+
85+
expect(nameInput.value).toBe('fluffy');
86+
expect(ageInput.value).toBe('3');
87+
expect(fixture.componentInstance.f().value().species).toBe('cat');
88+
});
89+
});
90+
91+
function act<T>(fn: () => T): T {
92+
try {
93+
return fn();
94+
} finally {
95+
TestBed.tick();
96+
}
97+
}

0 commit comments

Comments
 (0)