Merge pull request #30 from cloudnc/fix/do-not-call-fn-of-undefined-obj

fix(lib): do not call this.formGroup.updateValueAndValidity if form has been destroyed
cloudnc · Apr 1, 2019 · ccb248d · ccb248d
2 parents 7c6e01e + 686a2e7
commit ccb248d
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 8 deletions.
diff --git a/projects/ngx-sub-form/src/lib/ngx-sub-form.component.spec.ts b/projects/ngx-sub-form/src/lib/ngx-sub-form.component.spec.ts
@@ -19,7 +19,7 @@ const getDefaultValues = (): Required<Vehicle> => ({
 });
 
 class SubComponent extends NgxSubFormComponent<Vehicle> {
-  getFormControls() {
+  protected getFormControls() {
     // even though optional, if we comment out color there should be a TS error
     return {
       color: new FormControl(getDefaultValues().color),
@@ -32,6 +32,21 @@ class SubComponent extends NgxSubFormComponent<Vehicle> {
   }
 }
 
+describe(`Common`, () => {
+  it(`should call formGroup.updateValueAndValidity only if formGroup is defined`, (done: () => void) => {
+    const subComponent: SubComponent = new SubComponent();
+
+    const formGroupSpy = spyOn(subComponent.formGroup, 'updateValueAndValidity');
+
+    expect(formGroupSpy).not.toHaveBeenCalled();
+
+    setTimeout(() => {
+      expect(formGroupSpy).toHaveBeenCalled();
+      done();
+    }, 0);
+  });
+});
+
 describe(`NgxSubFormComponent`, () => {
   let subComponent: SubComponent;
 

diff --git a/projects/ngx-sub-form/src/lib/ngx-sub-form.component.ts b/projects/ngx-sub-form/src/lib/ngx-sub-form.component.ts
@@ -7,11 +7,14 @@ import { ControlMap, Controls, ControlsNames } from './ngx-sub-form-utils';
 export abstract class NgxSubFormComponent<ControlInterface, FormInterface = ControlInterface>
   implements ControlValueAccessor, Validator, OnDestroy {
   public get formGroupControls(): ControlMap<FormInterface, AbstractControl> {
-    return this.mapControls();
+    // @note form-group-undefined we need the as syntax here because we do not want to expose the fact that
+    // the form can be undefined, it's hanlded internally to contain an Angular bug
+    return this.mapControls() as ControlMap<FormInterface, AbstractControl>;
   }
 
   public get formGroupValues(): Required<FormInterface> {
-    return this.mapControls(ctrl => ctrl.value);
+    // see @note form-group-undefined for as syntax
+    return this.mapControls(ctrl => ctrl.value) as Required<FormInterface>;
   }
 
   public get formGroupErrors(): null | Partial<ControlMap<FormInterface, ValidationErrors | null>> {
@@ -20,17 +23,21 @@ export abstract class NgxSubFormComponent<ControlInterface, FormInterface = Cont
       ctrl => ctrl.invalid,
     );
 
-    if (!Object.keys(errors).length) {
+    if (!errors || !Object.keys(errors).length) {
       return null;
     }
 
     return errors;
   }
 
   public get formControlNames(): ControlsNames<FormInterface> {
-    return this.mapControls((_, key) => key);
+    // see @note form-group-undefined for as syntax
+    return this.mapControls((_, key) => key) as ControlsNames<FormInterface>;
   }
 
+  // when developing the lib it's a good idea to set the formGroup type
+  // to current + `| undefined` to catch a bunch of possible issues
+  // see @note form-group-undefined
   public formGroup: FormGroup & { controls: Controls<FormInterface> } = new FormGroup(this.getFormControls()) as any;
 
   protected onChange: Function | undefined = undefined;
@@ -48,14 +55,20 @@ export abstract class NgxSubFormComponent<ControlInterface, FormInterface = Cont
     // are not yet defined for the field `assassinDroid`
     // (until you change one of the value in that form)
     setTimeout(() => {
-      this.formGroup.updateValueAndValidity({ emitEvent: false });
+      if (this.formGroup) {
+        this.formGroup.updateValueAndValidity({ emitEvent: false });
+      }
     }, 0);
   }
 
   private mapControls<MapValue, T extends ControlMap<FormInterface, MapValue>>(
     mapControl?: (ctrl: Controls<FormInterface>[keyof FormInterface], key: keyof FormInterface) => MapValue,
     filterControl: (ctrl: Controls<FormInterface>[keyof FormInterface]) => boolean = () => true,
-  ): T {
+  ): T | null {
+    if (!this.formGroup) {
+      return null;
+    }
+
     const formControls: Controls<FormInterface> = this.formGroup.controls;
 
     if (!mapControl) {
@@ -147,7 +160,7 @@ export abstract class NgxSubFormComponent<ControlInterface, FormInterface = Cont
     // this is required to correctly initialize the form value
     // see note on-change-after-one-tick within the test file for more info
     setTimeout(() => {
-      if (this.onChange) {
+      if (this.onChange && this.formGroup) {
         this.onChange(this.transformFromFormGroup(this.formGroup.value));
       }
     }, 0);