From f18e1739b830d43131df0849bce49ffdd9dc6154 Mon Sep 17 00:00:00 2001 From: Dylan Hunn Date: Fri, 3 Jun 2022 14:50:36 -0700 Subject: [PATCH] fix(forms): allow FormBuilder.group(...) to accept optional fields. (#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; address?: FormControl; }> = fb.group<{ name: FormControl; address?: FormControl; }>(controls); ``` Today, with fully strict TypeScript settings, the above will not compile: ``` Types of property 'controls' are incompatible. Type '{ name: FormControl; address?: FormControl | null | undefined> | undefined; }' is not assignable to type '{ name: FormControl; address?: FormGroup | undefined; }'. ``` Notice that the `fb.group(...)` is calculating the following type for address: `address?: FormControl`. 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}` implicitly makes the RHS have type `FormControl|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`, not `FormGroup|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 --- .../forms_reactive/bundle.golden_symbols.json | 8 ++++---- .../bundle.golden_symbols.json | 8 ++++---- packages/forms/src/form_builder.ts | 15 +++++++++++++++ packages/forms/test/typed_integration_spec.ts | 8 ++++++++ 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json index 4f349bcdd11..185bd53349c 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -548,9 +548,6 @@ { "name": "_randomChar" }, - { - "name": "componentDefCount" - }, { "name": "_symbolIterator" }, @@ -656,6 +653,9 @@ { "name": "collectStylingFromTAttrs" }, + { + "name": "componentDefCount" + }, { "name": "compose" }, @@ -1595,4 +1595,4 @@ { "name": "ɵɵtext" } -] +] \ No newline at end of file diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json index be9a29f5f1e..4a829dafabb 100644 --- a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json @@ -539,9 +539,6 @@ { "name": "_randomChar" }, - { - "name": "componentDefCount" - }, { "name": "_symbolIterator" }, @@ -638,6 +635,9 @@ { "name": "collectStylingFromTAttrs" }, + { + "name": "componentDefCount" + }, { "name": "composeAsyncValidators" }, @@ -1583,4 +1583,4 @@ { "name": "ɵɵtext" } -] +] \ No newline at end of file diff --git a/packages/forms/src/form_builder.ts b/packages/forms/src/form_builder.ts index d27b071772e..e9bce0cab7d 100644 --- a/packages/forms/src/form_builder.ts +++ b/packages/forms/src/form_builder.ts @@ -52,11 +52,26 @@ export type ɵElement = // 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 + // + // Identify FormControl container types. [T] extends [FormControl] ? FormControl : + // Or FormControl containers that are optional in their parent group. + [T] extends [FormControl|undefined] ? FormControl : + // FormGroup containers. [T] extends [FormGroup] ? FormGroup : + // Optional FormGroup containers. + [T] extends [FormGroup|undefined] ? FormGroup : + // FormArray containers. [T] extends [FormArray] ? FormArray : + // Optional FormArray containers. + [T] extends [FormArray|undefined] ? FormArray : + // Otherwise unknown AbstractControl containers. [T] extends [AbstractControl] ? AbstractControl : + // Optional AbstractControl containers. + [T] extends [AbstractControl|undefined] ? AbstractControl : + // FormControlState object container, which produces a nullable control. [T] extends [FormControlState] ? FormControl : + // A ControlConfig tuple, which produces a nullable control. [T] extends [ControlConfig] ? FormControl : // 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 diff --git a/packages/forms/test/typed_integration_spec.ts b/packages/forms/test/typed_integration_spec.ts index b6a3dd24c72..21302856974 100644 --- a/packages/forms/test/typed_integration_spec.ts +++ b/packages/forms/test/typed_integration_spec.ts @@ -1032,6 +1032,14 @@ describe('Typed Class', () => { } }); + it('from objects with optional keys', () => { + const controls = {name: fb.control('')}; + const foo: + FormGroup<{name: FormControl; address?: FormControl;}> = + fb.group<{name: FormControl; address?: FormControl;}>( + controls); + }); + it('from objects with FormControlState', () => { const c = fb.group({foo: {value: 'bar', disabled: false}}); {