Skip to content

fix(forms): allow FormBuilder.group(...) to accept optional fields.#46253

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

fix(forms): allow FormBuilder.group(...) to accept optional fields.#46253
dylhunn wants to merge 1 commit intoangular:mainfrom
dylhunn:fb-group-optional

Conversation

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Jun 3, 2022

Consider the case in which FormBuilder is used to construct a group with an optional field:

const controls = { name: fb.control('') };
const foo: FormGroup<{
  name: FormControl<string | null>;
  address?: FormControl<string | null>;
}> = fb.group<{
  name: FormControl<string | null>;
  address?: FormControl<string | null>;
}>(controls);

Today, with fully strict TypeScript settings, the above will not compile:

Types of property 'controls' are incompatible.
Type '{ name: FormControl<string | null>; address?: FormControl<FormGroup<SubFormControls> | null | undefined> | undefined; }' is not assignable to type '{ name: FormControl<string | null>; address?: FormGroup<SubFormControls> | undefined; }'.

Notice that the fb.group(...) is calculating the following type for address: address?: FormControl<FormGroup<string|null>. This is clearly wrong -- an extraneous FormControl has been added!

This is coming from the calculation of the result type of fb.group(...). In the type definition, if we cannot detect the outer control type, we assume it's just an unwrapped value, and automatically wrap it in FormControl.

Because the optional {address?: FormControl<string|null>} implicitly makes the RHS have type FormControl<string|null>|undefined, the relevant condition is not satisfied. In particular, the condition expects just FormGroup<T>, not FormGroup<T>|undefined. So we assume T is a value type, and it gets wrapped with FormControl.

The solution is to add the cases where undefined is included in the union type when detecting which control T is (if any).

Google-internal bug: b/234856858

@dylhunn dylhunn added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer area: forms target: patch This PR is targeted for the next patch release action: presubmit The PR is in need of a google3 presubmit cross-cutting: types P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful forms: strictly typed labels Jun 3, 2022
@dylhunn dylhunn requested a review from AndrewKushnir June 3, 2022 22:00
@ngbot ngbot bot modified the milestone: Backlog Jun 3, 2022
@dylhunn dylhunn force-pushed the fb-group-optional branch 2 times, most recently from 1fc488c to 91e6fe2 Compare June 3, 2022 22:02
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Just a quick comment on adding more docs (which can also be done in a followup PR).

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 3, 2022
Consider the case in which `FormBuilder` is used to construct a group with an optional field:

```
const controls = { name: fb.control('') };
const foo: FormGroup<{
  name: FormControl<string | null>;
  address?: FormControl<string | null>;
}> = fb.group<{
  name: FormControl<string | null>;
  address?: FormControl<string | null>;
}>(controls);
```

Today, with fully strict TypeScript settings, the above will not compile:

```
Types of property 'controls' are incompatible.
Type '{ name: FormControl<string | null>; address?: FormControl<FormGroup<SubFormControls> | null | undefined> | undefined; }' is not assignable to type '{ name: FormControl<string | null>; address?: FormGroup<SubFormControls> | undefined; }'.
```

Notice that the `fb.group(...)` is calculating the following type for address: `address?: FormControl<FormGroup<string|null>`. This is clearly wrong -- an extraneous `FormControl` has been added!

This is coming from the calculation of the result type of `fb.group(...)`. In the type definition, if we cannot detect the outer control type, [we assume it's just an unwrapped value, and automatically wrap it in `FormControl`](https://github.com/angular/angular/blob/14.0.0/packages/forms/src/form_builder.ts#L66).

Because the optional `{address?: FormControl<string|null>}` implicitly makes the RHS have type `FormControl<string|null>|undefined`, [the relevant condition is not satisfied](https://github.com/angular/angular/blob/14.0.0/packages/forms/src/form_builder.ts#L55). In particular, the condition expects just `FormGroup<T>`, not `FormGroup<T>|undefined`. So we assume `T` is a value type, and it gets wrapped with `FormControl`.

The solution is to add the cases where `undefined` is included in the union type when detecting which control `T` is (if any).
@dylhunn dylhunn force-pushed the fb-group-optional branch from 91e6fe2 to cb329d0 Compare June 4, 2022 00:18
@dylhunn
Copy link
Contributor Author

dylhunn commented Jun 4, 2022

@dylhunn dylhunn added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Jun 4, 2022
@alxhub
Copy link
Member

alxhub commented Jun 6, 2022

This PR was merged into the repository by commit f18e173.

@alxhub alxhub closed this in f18e173 Jun 6, 2022
alxhub pushed a commit that referenced this pull request Jun 6, 2022
…46253)

Consider the case in which `FormBuilder` is used to construct a group with an optional field:

```
const controls = { name: fb.control('') };
const foo: FormGroup<{
  name: FormControl<string | null>;
  address?: FormControl<string | null>;
}> = fb.group<{
  name: FormControl<string | null>;
  address?: FormControl<string | null>;
}>(controls);
```

Today, with fully strict TypeScript settings, the above will not compile:

```
Types of property 'controls' are incompatible.
Type '{ name: FormControl<string | null>; address?: FormControl<FormGroup<SubFormControls> | null | undefined> | undefined; }' is not assignable to type '{ name: FormControl<string | null>; address?: FormGroup<SubFormControls> | undefined; }'.
```

Notice that the `fb.group(...)` is calculating the following type for address: `address?: FormControl<FormGroup<string|null>`. This is clearly wrong -- an extraneous `FormControl` has been added!

This is coming from the calculation of the result type of `fb.group(...)`. In the type definition, if we cannot detect the outer control type, [we assume it's just an unwrapped value, and automatically wrap it in `FormControl`](https://github.com/angular/angular/blob/14.0.0/packages/forms/src/form_builder.ts#L66).

Because the optional `{address?: FormControl<string|null>}` implicitly makes the RHS have type `FormControl<string|null>|undefined`, [the relevant condition is not satisfied](https://github.com/angular/angular/blob/14.0.0/packages/forms/src/form_builder.ts#L55). In particular, the condition expects just `FormGroup<T>`, not `FormGroup<T>|undefined`. So we assume `T` is a value type, and it gets wrapped with `FormControl`.

The solution is to add the cases where `undefined` is included in the union type when detecting which control `T` is (if any).

PR Close #46253
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 17, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`14.0.0` -> `14.0.1`](https://renovatebot.com/diffs/npm/@angular%2fanimations/14.0.0/14.0.1) |
| [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`14.0.0` -> `14.0.1`](https://renovatebot.com/diffs/npm/@angular%2fcommon/14.0.0/14.0.1) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`14.0.0` -> `14.0.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/14.0.0/14.0.1) |
| [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | patch | [`14.0.0` -> `14.0.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/14.0.0/14.0.1) |
| [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`14.0.0` -> `14.0.1`](https://renovatebot.com/diffs/npm/@angular%2fcore/14.0.0/14.0.1) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`14.0.0` -> `14.0.1`](https://renovatebot.com/diffs/npm/@angular%2fforms/14.0.0/14.0.1) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`14.0.0` -> `14.0.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/14.0.0/14.0.1) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`14.0.0` -> `14.0.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/14.0.0/14.0.1) |

---

### Release Notes

<details>
<summary>angular/angular</summary>

### [`v14.0.1`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1401-2022-06-08)

[Compare Source](angular/angular@14.0.0...14.0.1)

##### bazel

| Commit | Type | Description |
| -- | -- | -- |
| [b00d237c0e](angular/angular@b00d237) | fix | update API extractor version ([#&#8203;46259](angular/angular#46259)) |
| [9a0a7bac21](angular/angular@9a0a7ba) | perf | reduce input files for `ng_package` rollup and type bundle actions ([#&#8203;46187](angular/angular#46187)) |

##### forms

| Commit | Type | Description |
| -- | -- | -- |
| [dde0b7f4b3](angular/angular@dde0b7f) | fix | allow FormBuilder.group(...) to accept optional fields. ([#&#8203;46253](angular/angular#46253)) |

#### Special Thanks

Adrien Crivelli, Alan Agius, Alex Rickabaugh, Andrew Kushnir, Andrew Scott, Dylan Hunn, Fabrizio Fallico, George Kalpakas, Jelle Bruisten, JoostK, Kristiyan Kostadinov, Krzysztof Platis, Paul Gschwendtner, Phalgun Vaddepalli, San Leen, dario-piotrowicz, mgechev and wellWINeo

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1404
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@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 Jul 7, 2022
@dylhunn dylhunn deleted the fb-group-optional branch October 17, 2022 13:38
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: strictly typed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants