Skip to content

fix(forms): Prevent FormBuilder from distributing unions to control types#45942

Closed
dylhunn wants to merge 1 commit intoangular:mainfrom
dylhunn:fb-dist
Closed

fix(forms): Prevent FormBuilder from distributing unions to control types#45942
dylhunn wants to merge 1 commit intoangular:mainfrom
dylhunn:fb-dist

Conversation

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented May 10, 2022

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. The handbook suggests a solution when distributivity is not wanted:

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 the above suggestion to FormBuilder's helper type.

Fixes #45912.

@dylhunn dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. cross-cutting: types target: rc This PR is targeted for the next release-candidate forms: strictly typed labels May 10, 2022
@ngbot ngbot bot added this to the Backlog milestone May 10, 2022
@dylhunn dylhunn requested a review from AndrewKushnir May 10, 2022 07:25
…ypes.

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 angular#45912.
@dylhunn dylhunn added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 10, 2022
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit e441ff4.

AndrewKushnir pushed a commit that referenced this pull request May 10, 2022
…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
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 10, 2022
@dylhunn dylhunn deleted the fb-dist branch November 30, 2022 20:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: forms cross-cutting: types forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. forms: strictly typed target: rc This PR is targeted for the next release-candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FormBuilder#group incorrectly infers type if union type is passed in

2 participants