Skip to content

Commit

Permalink
fix(forms): Prevent FormBuilder from distributing unions to control t…
Browse files Browse the repository at this point in the history
…ypes. (#45942)

Previously, using `FormBuilder` with a union type would produce unions of *controls*:

```
// `foo` has type `FormControl<string>|FormControl<number>`.
const c = fb.nonNullable.group({foo: 'bar' as string | number});
```

This actually works in many cases, due to how extraordinarily powerful Typescript's distributive types are (e.g. `value` still has type `string|number`), but it is subtly incorrect. Here is a code example that exposes the reason the inference is incorrect. It exploits the fact that Typescript will not "un-distribute" a type, producing an obviously spurious error:

```
// fc gets an inferred distributive union type `FormControl<string> | FormControl<number>`
let fc = c.controls.foo;
// Error: Type 'FormControl<string | number>' is not assignable to type 'FormControl<string> | FormControl<number>'.
fc = new FormControl<string|number>('', {initialValueIsDefault: true});
```

Instead, we want the union to apply to the *values*:

```
// `foo` should have type `FormControl<string|number>`.
const c = fb.nonNullable.group({foo: 'bar' as string | number});
```

Essentially, we want to prevent Typescript from distributing the type. [As specified in the handbook](https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types):

> Typically, distributivity is the desired behavior. To avoid that behavior, you can surround each side of the extends keyword with square brackets.

This PR applies this suggestion to `FormBuilder`'s type inference.

Fixes #45912.

PR Close #45942
  • Loading branch information
dylhunn authored and AndrewKushnir committed May 10, 2022
1 parent 74321a4 commit e441ff4
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 7 deletions.
17 changes: 10 additions & 7 deletions packages/forms/src/form_builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,19 @@ export type ControlConfig<T> = [T|FormControlState<T>, (ValidatorFn|(ValidatorFn
* The flag N, if not never, makes the resulting `FormControl` have N in its type.
*/
export type ɵElement<T, N extends null> =
T extends FormControl<infer U> ? FormControl<U> :
T extends FormGroup<infer U> ? FormGroup<U> :
T extends FormArray<infer U> ? FormArray<U> :
T extends AbstractControl<infer U> ? AbstractControl<U> :
T extends FormControlState<infer U> ? FormControl<U|N> :
T extends ControlConfig<infer U> ? FormControl<U|N> :
// The `extends` checks are wrapped in arrays in order to prevent TypeScript from applying type unions
// through the distributive conditional type. This is the officially recommended solution:
// https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types
[T] extends [FormControl<infer U>] ? FormControl<U> :
[T] extends [FormGroup<infer U>] ? FormGroup<U> :
[T] extends [FormArray<infer U>] ? FormArray<U> :
[T] extends [AbstractControl<infer U>] ? AbstractControl<U> :
[T] extends [FormControlState<infer U>] ? FormControl<U|N> :
[T] extends [ControlConfig<infer U>] ? FormControl<U|N> :
// ControlConfig can be too much for the compiler to infer in the wrapped case. This is
// not surprising, since it's practically death-by-polymorphism (e.g. the optional validators
// members that might be arrays). Watch for ControlConfigs that might fall through.
T extends Array<infer U|ValidatorFn|ValidatorFn[]|AsyncValidatorFn|AsyncValidatorFn[]> ? FormControl<U|N> :
[T] extends [Array<infer U|ValidatorFn|ValidatorFn[]|AsyncValidatorFn|AsyncValidatorFn[]>] ? FormControl<U|N> :
// Fallthough case: T is not a container type; use it directly as a value.
FormControl<T|N>;

Expand Down
12 changes: 12 additions & 0 deletions packages/forms/test/typed_integration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,18 @@ describe('Typed Class', () => {
expect(c.value).toEqual({foo: 'bar'});
});

it('without distributing union types', () => {
const c = fb.group({foo: 'bar' as string | number});
{
type ControlsType = {foo: FormControl<string|number>};
let t: ControlsType = c.controls;
let t1 = c.controls;
t1 = null as unknown as ControlsType;
}
let fc = c.controls.foo;
fc = new FormControl<string|number>('', {initialValueIsDefault: true});
});

describe('from objects with FormControls', () => {
it('from objects with builder FormGroups', () => {
const c = fb.group({foo: fb.group({baz: 'bar'})});
Expand Down

0 comments on commit e441ff4

Please sign in to comment.