fix(forms): fix FormRecord type inference - #50752

cc @dylhunn You might wanna take a look at this 👍

Up 🙂 @dylhunn

Interesting. Can you show an example of an inference that would have been broken under the previous regime? It would be good to add it to typed_integration_spec as well, to document the change and prevent regressions.

@dylhunn you can take a look at the failed test in this PR : #50750

A FormRecord is inferred as FormGroup.
While this is not incorrect technically, because FormRecord actually extends FormGroup (and this is the reason of bad inference), it is probably unexpected.
The helper type ɵElement should probably infer it to FormRecord instead.

@EmmanuelRoux If this PR provides a fix it should also introduce a previously broken test !

@JeanMeche This is why I provided a link to #50750 :

Currently, this does NOT break anything, because FormRecord type is actually compatible with FormGroup (and vice versa). See :

type X = ɵElement<FormRecord<FormControl<string>>, null>;
// X is currently inferred as FormGroup<{[p: string]: FormControl<string>}>

const formGroupX: X = null!;
const formGroupY: FormGroup<{[p: string]: FormControl<string>}> = a; 
const formRecordZ: FormRecord<FormControl<string>> = a; // <-- Currently no error, because FormGroup can be assigned to FormRecord

BUT, if FormRecord would define some specific signature it would become incompatible with FormGroup:

// Let's imagine FormRecord has a new method, for example:
export interface FormRecord<TControl> {
  // ...
  someMethod(): TControl;
}

// Then this would break (as expected) :

const formGroupX: X = null!;
const formGroupY: FormGroup<{[p: string]: FormControl<string>}> = a; 
const formRecordZ: FormRecord<FormControl<string>> = a; // <-- Error, because FormGroup cannot be assigned to FormRecord

This is exactly what the PR #50750 aims to introduce.

This is for the sake of commit tree grooming. That we'd like to have tests covering bugfixes / feature introductions.

Either this PR should introduce new test, which shows the fix its bringing or the PR should be merged with #50750.

@JeanMeche @dylhunn merged with #50750

Any update on this PR or #50750 please ?

I'll close this in favor of #50750 which is getting reviewed and tested.

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}