Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

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
…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