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

dylhunn · alxhub · commit f18e1739b830 · 2022-06-06T10:14:19.000-07:00
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
diff --git 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"
   }
-]
+]
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
@@ -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"
   }
-]
+]
diff --git a/packages/forms/src/form_builder.ts b/packages/forms/src/form_builder.ts
@@ -52,11 +52,26 @@ export type ɵElement<T, N extends null> =
   // 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<infer U>] ? FormControl<U> :
+  // Or FormControl containers that are optional in their parent group.
+  [T] extends [FormControl<infer U>|undefined] ? FormControl<U> :
+  // FormGroup containers.
   [T] extends [FormGroup<infer U>] ? FormGroup<U> :
+  // Optional FormGroup containers.
+  [T] extends [FormGroup<infer U>|undefined] ? FormGroup<U> :
+  // FormArray containers.
   [T] extends [FormArray<infer U>] ? FormArray<U> :
+  // Optional FormArray containers.
+  [T] extends [FormArray<infer U>|undefined] ? FormArray<U> :
+  // Otherwise unknown AbstractControl containers.
   [T] extends [AbstractControl<infer U>] ? AbstractControl<U> :
+  // Optional AbstractControl containers.
+  [T] extends [AbstractControl<infer U>|undefined] ? AbstractControl<U> :
+  // FormControlState object container, which produces a nullable control.
   [T] extends [FormControlState<infer U>] ? FormControl<U|N> :
+  // A ControlConfig tuple, which produces a nullable control.
   [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
diff --git 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<string|null>; address?: FormControl<string|null>;}> =
+                fb.group<{name: FormControl<string|null>; address?: FormControl<string|null>;}>(
+                    controls);
+      });
+
       it('from objects with FormControlState', () => {
         const c = fb.group({foo: {value: 'bar', disabled: false}});
         {

Original file line number	Diff line number	Diff line change
`@@ -548,9 +548,6 @@`
`548`	`548`	`{`
`549`	`549`	`"name": "_randomChar"`
`550`	`550`	`},`
`551`		`- {`
`552`		`- "name": "componentDefCount"`
`553`		`- },`
`554`	`551`	`{`
`555`	`552`	`"name": "_symbolIterator"`
`556`	`553`	`},`
`@@ -656,6 +653,9 @@`
`656`	`653`	`{`
`657`	`654`	`"name": "collectStylingFromTAttrs"`
`658`	`655`	`},`
	`656`	`+ {`
	`657`	`+ "name": "componentDefCount"`
	`658`	`+ },`
`659`	`659`	`{`
`660`	`660`	`"name": "compose"`
`661`	`661`	`},`
`@@ -1595,4 +1595,4 @@`
`1595`	`1595`	`{`
`1596`	`1596`	`"name": "ɵɵtext"`
`1597`	`1597`	`}`
`1598`		`-]`
	`1598`	`+]`
Original file line number	Diff line number	Diff line change
`@@ -539,9 +539,6 @@`
`539`	`539`	`{`
`540`	`540`	`"name": "_randomChar"`
`541`	`541`	`},`
`542`		`- {`
`543`		`- "name": "componentDefCount"`
`544`		`- },`
`545`	`542`	`{`
`546`	`543`	`"name": "_symbolIterator"`
`547`	`544`	`},`
`@@ -638,6 +635,9 @@`
`638`	`635`	`{`
`639`	`636`	`"name": "collectStylingFromTAttrs"`
`640`	`637`	`},`
	`638`	`+ {`
	`639`	`+ "name": "componentDefCount"`
	`640`	`+ },`
`641`	`641`	`{`
`642`	`642`	`"name": "composeAsyncValidators"`
`643`	`643`	`},`
`@@ -1583,4 +1583,4 @@`
`1583`	`1583`	`{`
`1584`	`1584`	`"name": "ɵɵtext"`
`1585`	`1585`	`}`
`1586`		`-]`
	`1586`	`+]`
Original file line number	Diff line number	Diff line change
`@@ -1032,6 +1032,14 @@ describe('Typed Class', () => {`
`1032`	`1032`	`}`
`1033`	`1033`	`});`
`1034`	`1034`
	`1035`	`+ it('from objects with optional keys', () => {`
	`1036`	`+ const controls = {name: fb.control('')};`
	`1037`	`+ const foo:`
	`1038`	`+ FormGroup<{name: FormControl<string\|null>; address?: FormControl<string\|null>;}> =`
	`1039`	`+ fb.group<{name: FormControl<string\|null>; address?: FormControl<string\|null>;}>(`
	`1040`	`+ controls);`
	`1041`	`+ });`
	`1042`	`+`
`1035`	`1043`	`it('from objects with FormControlState', () => {`
`1036`	`1044`	`const c = fb.group({foo: {value: 'bar', disabled: false}});`
`1037`	`1045`	`{`