Forms compat Attempt 2 #64861
Forms compat Attempt 2 #64861
Conversation
kirjs
force-pushed
the
forms-compat-2
branch
3 times, most recently
from
dea1fcb to
4f1c571
Compare
kirjs
force-pushed
the
forms-compat-2
branch
7 times, most recently
from
648aed0 to
d543664
Compare
kirjs
requested review from
leonsenft and
mmalerba
and removed request for
crisbeto and
thePunderWoman
| export type Subfields<TValue> = { | ||
| readonly [K in keyof TValue as TValue[K] extends Function ? never : K]: MaybeFieldTree< | ||
| TValue[K], | ||
| export type Subfields<TModel> = { |
There was a problem hiding this comment.
Remind me what was the distinction between TModel and TValue?
I thought TModel was to be used when the value might be either the field value or a AbstractControl? In this case, shouldn't this be TValue?
There was a problem hiding this comment.
It looks like you changed TValue to TModel elsewhere for consistency which seems right, but unless my mental model is incorrect, this should actually be TValue here because if we're dealing with subfields, we know it's not a compat field, right?
There was a problem hiding this comment.
Yes, looks like it becomes TValue here for a second, good catch
| * An error used for compat errors. | ||
| */ | ||
| export class CompatValidationError<T = unknown> implements ValidationError { | ||
| readonly kind: string = 'reactive'; |
There was a problem hiding this comment.
Does it make sense for the kind of error here to be reactive instead of compat? Since we use the term compat everywhere instead of reactive it might be more clear.
There was a problem hiding this comment.
Resolved but unchanged. Do you disagree?
There was a problem hiding this comment.
I vote for consistently using the term "compat". I don't like "reactive" because signals are reactive, and in the context of signal forms, that's the first thing I think of, not reactive forms. but whichever term we choose, we should be consistent about it
kirjs
force-pushed
the
forms-compat-2
branch
3 times, most recently
from
e6c2183 to
7686936
Compare
This allows using reactive form controls in signal forms
| export type Subfields<TValue> = { | ||
| readonly [K in keyof TValue as TValue[K] extends Function ? never : K]: MaybeFieldTree< | ||
| TValue[K], | ||
| export type Subfields<TModel> = { |
There was a problem hiding this comment.
It looks like you changed TValue to TModel elsewhere for consistency which seems right, but unless my mental model is incorrect, this should actually be TValue here because if we're dealing with subfields, we know it's not a compat field, right?
| * An error used for compat errors. | ||
| */ | ||
| export class CompatValidationError<T = unknown> implements ValidationError { | ||
| readonly kind: string = 'reactive'; |
There was a problem hiding this comment.
Resolved but unchanged. Do you disagree?
kirjs
force-pushed
the
forms-compat-2
branch
2 times, most recently
from
0538a83 to
a8a42a7
Compare
Now if a field contains form control, it'll unwrap its type correctly
This is more aligned with the way signal forms work, and spares user from having to deal with partial types.
|
Deployed adev-preview for 73cf1c9 to: https://ng-dev-previews-fw--pr-angular-angular-64861-adev-prev-diq1qj7n.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
3 participants
In this PR we're using the same types for both compat and regular forms