feat(signal-forms): Add key & index support in FieldContext #62347
Conversation
mmalerba
added
area: forms
target: feature
| ? {[K in keyof T]: ChildFieldPath<T[K]>} | ||
| : {}); | ||
| export type ChildFieldPath<T> = { | ||
| [ɵɵTYPE]: [T, 'child']; |
There was a problem hiding this comment.
What is the purpose of this ɵɵTYPE property?
/**
* Symbol used to retain generic type information when it would otherwise be lost.
*/
declare const ɵɵTYPE: unique symbol;The definition makes it clear that T helps retain type information, but how is the 'child' or 'item' used?
| const f = form( | ||
| cat, | ||
| (p) => { | ||
| //validate(p, recordKey); |
There was a problem hiding this comment.
Is this line supposed to be tested? I see there's a related comment in the expectation.
| const f = form( | ||
| pets, | ||
| (p) => { | ||
| //validate(p, recordIndex); |
| } & (T extends any[] | ||
| ? {} | ||
| : T extends Record<PropertyKey, any> | ||
| ? {[K in keyof T]: FieldPath<T[K]>} | ||
| ? {[K in keyof T]: ChildFieldPath<T[K]>} |
There was a problem hiding this comment.
Is it possible or useful to define the elements of T when T extends any[] as ItemFieldPaths?
T extends any[]
? {[index: number]: ItemFieldPath<T[number]>}
: T extends Record<PropertyKey, any>
? {[K in keyof T]: ChildFieldPath<T[K]>}
: {}Or is this an issue due to the dynamic nature of an array, because you don't know which indices are valid?
| path: FieldPath<T> | ChildFieldPath<T> | ItemFieldPath<T>, | ||
| logic: NoInfer<Validator<T> | ChildValidator<T> | ItemValidator<T>>, | ||
| ): void { | ||
| assertPathIsCurrent(path as FieldPath<T>); |
There was a problem hiding this comment.
Is this as cast necessary? Don't all of the types in the union implement this type?
| @@ -181,11 +184,14 @@ export function form<T>(...args: any[]): Field<T> { | |||
| * element of the array. | |||
| * @template T The data type of an element in the array. | |||
| */ | |||
| export function applyEach<T>(path: FieldPath<T[]>, schema: NoInfer<SchemaOrSchemaFn<T>>): void { | |||
| assertPathIsCurrent(path); | |||
| export function applyEach<T>( | |||
There was a problem hiding this comment.
Is the array() in the comment @example meant to be applyEach()?
| export function applyEach<T>(path: FieldPath<T[]>, schema: NoInfer<SchemaOrSchemaFn<T>>): void { | ||
| assertPathIsCurrent(path); | ||
| export function applyEach<T>( | ||
| path: FieldPath<T[]> | ChildFieldPath<T[]> | ItemFieldPath<T[]>, |
There was a problem hiding this comment.
Does the union actually matter here? I'm guessing it's just meant to accept any type of field path, but can we define ChildFieldPath and ItemFieldPath to just satisfy FieldPath? That way the distinction would only matter in cases where you wish to explicitly read key() or index().
| readonly index = computed(() => { | ||
| const key = this.key(); | ||
| if (!Array.isArray(untracked(this.node.structure.parent!.value))) { | ||
| throw new Error(`RuntimeError: cannot access index, parent field is not an array`); |
There was a problem hiding this comment.
I know core has a system for runtime errors that supports "minifying" them in release mode. Is that something utilized by other Angular packages (e.g. forms, router) or only core?
| @@ -199,7 +199,7 @@ export class ChildFieldNodeStructure extends FieldNodeStructure { | |||
| } | |||
|
|
|||
| // Check the parent value at the last known key to avoid a scan. | |||
| const data = parentValue[lastKnownKey as number]; | |||
| const data = parentValue[lastKnownKey as unknown as number]; | |||
There was a problem hiding this comment.
Am I understanding correctly that this works because array[1] === array['1']? Is this better than Number(lastKnownKey)?
TODO: try this and see if its worth the added type complexity: