From 4d66185cbcb42c7e1b959f3b7ec9c34e9ef08952 Mon Sep 17 00:00:00 2001
From: Andrew Kushnir <akushnir@google.com>
Date: Fri, 22 Jan 2021 10:59:36 -0800
Subject: [PATCH] fix(forms): properly cleanup in cases when FormControlName
 has no CVA (#40526)

PR #39235 introduced additional cleanup logic for form controls and directives. The cleanup logic relies
on the presence of ControlValueAccessor instances on FormControlName and FormControl directives. In general
these fields are present and there are also checks to make sure that the mentioned directive instances are
created with CVAs. However some scenarios (primarily tests) may invoke the logic in a way that the directive
instance would not be fully initialized, thus causing CVA to be absent. As a result, the cleanup logic fails
while trying to call some methods on associated CVA instances.

This commit updates the cleanup logic to take into account the situation when CVA is not present.

Fixes #40521.

PR Close #40526
---
 packages/forms/src/directives/shared.ts       | 11 ++++++--
 .../forms/test/reactive_integration_spec.ts   | 28 ++++++++++++++++++-
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/packages/forms/src/directives/shared.ts b/packages/forms/src/directives/shared.ts
index 6d4d6441ff..17c0a1d75a 100644
--- a/packages/forms/src/directives/shared.ts
+++ b/packages/forms/src/directives/shared.ts
@@ -75,8 +75,15 @@ export function cleanUpControl(
     }
   };
 
-  dir.valueAccessor!.registerOnChange(noop);
-  dir.valueAccessor!.registerOnTouched(noop);
+  // The `valueAccessor` field is typically defined on FromControl and FormControlName directive
+  // instances and there is a logic in `selectValueAccessor` function that throws if it's not the
+  // case. We still check the presence of `valueAccessor` before invoking its methods to make sure
+  // that cleanup works correctly if app code or tests are setup to ignore the error thrown from
+  // `selectValueAccessor`. See https://github.com/angular/angular/issues/40521.
+  if (dir.valueAccessor) {
+    dir.valueAccessor.registerOnChange(noop);
+    dir.valueAccessor.registerOnTouched(noop);
+  }
 
   cleanUpValidators(control, dir, /* handleOnValidatorChange */ true);
 
diff --git a/packages/forms/test/reactive_integration_spec.ts b/packages/forms/test/reactive_integration_spec.ts
index 379ef229b9..fcdb1c6c45 100644
--- a/packages/forms/test/reactive_integration_spec.ts
+++ b/packages/forms/test/reactive_integration_spec.ts
@@ -7,7 +7,7 @@
  */
 
 import {ɵgetDOM as getDOM} from '@angular/common';
-import {Component, Directive, forwardRef, Input, OnDestroy, Type} from '@angular/core';
+import {Component, Directive, forwardRef, Input, NgModule, OnDestroy, Type} from '@angular/core';
 import {ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing';
 import {expect} from '@angular/core/testing/src/testing_internal';
 import {AbstractControl, AsyncValidator, AsyncValidatorFn, COMPOSITION_BUFFER_MODE, ControlValueAccessor, DefaultValueAccessor, FormArray, FormControl, FormControlDirective, FormControlName, FormGroup, FormGroupDirective, FormsModule, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, ReactiveFormsModule, Validator, Validators} from '@angular/forms';
@@ -4127,6 +4127,32 @@ const ValueAccessorB = createControlValueAccessor('[cva-b]');
           valueChanges: {group: {control: 'Updated value'}},
         });
       });
+
+      // See https://github.com/angular/angular/issues/40521.
+      it('should properly clean up when FormControlName has no CVA', () => {
+        @Component({
+          selector: 'no-cva-compo',
+          template: `
+            <form [formGroup]="form">
+              <div formControlName="control"></div>
+            </form>
+          `
+        })
+        class NoCVAComponent {
+          form = new FormGroup({control: new FormControl()});
+        }
+
+        const fixture = initTest(NoCVAComponent);
+        expect(() => {
+          fixture.detectChanges();
+        }).toThrowError('No value accessor for form control with name: \'control\'');
+
+        // Making sure that cleanup between tests doesn't cause any issues
+        // for not fully initialized controls.
+        expect(() => {
+          fixture.destroy();
+        }).not.toThrow();
+      });
     });
   });
 }