Merge pull request #20 from cloudnc/fix/on-change-callback-after-one-…

…tick

fix(lib): do not call onChange callback when the component is created, wait for one tick before doing so

projects/ngx-sub-form/src/lib/ngx-sub-form.component.spec.ts

@@ -172,15 +172,26 @@ describe(`NgxSubFormComponent`, () => {
   });
 
   describe(`value updated by the sub form (onChange)`, () => {
-    it(`should call onChange callback as soon as it's being registered`, () => {
+    // @note on-change-after-one-tick
+    // we need to wait for one tick otherwise it might in certain case trigger an error
+    // `ExpressionChangedAfterItHasBeenCheckedError`
+    // see issue here: https://github.com/cloudnc/ngx-sub-form/issues/15
+    // repro here: https://github.com/lppedd/ngx-sub-form-test
+    // stackblitz here: https://stackblitz.com/edit/ngx-sub-form-repro-issue-15 (might have to download, seems broken on stackblitz)
+    it(`should call onChange callback as soon as it's being registered (after one tick)`, (done: () => void) => {
       const spy = jasmine.createSpy();
       subComponent.registerOnChange(spy);
 
-      expect(spy).toHaveBeenCalledTimes(1);
-      expect(spy).toHaveBeenCalledWith(getDefaultValues());
+      expect(spy).not.toHaveBeenCalled();
+
+      setTimeout(() => {
+        expect(spy).toHaveBeenCalledTimes(1);
+        expect(spy).toHaveBeenCalledWith(getDefaultValues());
+        done();
+      }, 0);
     });
 
-    it(`should call onChange and onTouched callback on next tick every time the form value changes`, (done: () => void) => {
+    it(`should call onChange and onTouched callback without waiting for next tick every time the form value changes`, () => {
       const onTouchedSpy = jasmine.createSpy('onTouchedSpy');
       const onChangeSpy = jasmine.createSpy('onChangeSpy');
 
@@ -189,13 +200,9 @@ describe(`NgxSubFormComponent`, () => {
 
       subComponent.formGroup.setValue(getDefaultValues());
 
-      setTimeout(() => {
-        expect(onTouchedSpy).toHaveBeenCalledTimes(1);
-        expect(onChangeSpy).toHaveBeenCalledTimes(2);
-        expect(onChangeSpy).toHaveBeenCalledWith(getDefaultValues());
-
-        done();
-      }, 0);
+      expect(onTouchedSpy).toHaveBeenCalledTimes(1);
+      expect(onChangeSpy).toHaveBeenCalledTimes(1);
+      expect(onChangeSpy).toHaveBeenCalledWith(getDefaultValues());
     });
   });
 });
@@ -267,7 +274,8 @@ describe(`NgxSubFormRemapComponent`, () => {
   });
 
   describe(`value updated by the sub form (onChange)`, () => {
-    it(`should call onChange callback with the formValue transformed by the transformFromFormGroup method`, (done: () => void) => {
+    // about the after one tick, see note on-change-after-one-tick
+    it(`should call onChange callback with the formValue transformed by the transformFromFormGroup method (after one tick)`, (done: () => void) => {
       const onChangeSpy = jasmine.createSpy('onChangeSpy');
 
       subRemapComponent.registerOnChange(onChangeSpy);
@@ -278,19 +286,21 @@ describe(`NgxSubFormRemapComponent`, () => {
         numberOfPeopleOnBoard: getDefaultValues().numberOfPeopleOnBoard,
       };
 
-      expect(onChangeSpy).toHaveBeenCalledWith(expectedValue);
-
-      onChangeSpy.calls.reset();
-
-      subRemapComponent.formGroup.setValue({
-        vehiculeColor: getDefaultValues().color,
-        vehiculeCanFire: getDefaultValues().canFire,
-        vehiculeNumberOfPeopleOnBoard: getDefaultValues().numberOfPeopleOnBoard,
-      });
+      expect(onChangeSpy).not.toHaveBeenCalled();
 
       setTimeout(() => {
         expect(onChangeSpy).toHaveBeenCalledWith(expectedValue);
 
+        onChangeSpy.calls.reset();
+
+        subRemapComponent.formGroup.setValue({
+          vehiculeColor: getDefaultValues().color,
+          vehiculeCanFire: getDefaultValues().canFire,
+          vehiculeNumberOfPeopleOnBoard: getDefaultValues().numberOfPeopleOnBoard,
+        });
+
+        // this one shouldn't be async
+        expect(onChangeSpy).toHaveBeenCalledWith(expectedValue);
         done();
       }, 0);
     });

projects/ngx-sub-form/src/lib/ngx-sub-form.component.ts

@@ -1,7 +1,7 @@
 import { OnDestroy } from '@angular/core';
 import { ControlValueAccessor, FormGroup, ValidationErrors, Validator } from '@angular/forms';
 import { Subscription } from 'rxjs';
-import { delay, tap } from 'rxjs/operators';
+import { tap } from 'rxjs/operators';
 import { Controls, ControlsNames, getControlsNames } from './ngx-sub-form-utils';
 
 export abstract class NgxSubFormComponent<ControlInterface, FormInterface = ControlInterface>
@@ -113,14 +113,23 @@ export abstract class NgxSubFormComponent<ControlInterface, FormInterface = Cont
     this.onChange = fn;
 
     // this is required to correctly initialize the form value
-    this.onChange(this.transformFromFormGroup(this.formGroup.value));
+    // see note on-change-after-one-tick within the test file for more info
+    // it makes sense to have it on the next tick and  without delay when the formGroup
+    // changes because otherwise it's breaking the one way data flow (and we've got an error expression has changed...):
+    // parent creates a sub form, it does call `registerOnChange`, we trigger a change because the ouput might not match
+    // the input (if implementing `transformToFormGroup`/`transformFromFormGroup`), value on the parent will be updated
+    setTimeout(() => {
+      if (this.onChange) {
+        this.onChange(this.transformFromFormGroup(this.formGroup.value));
+      }
+    }, 0);
 
     this.subscription = this.formGroup.valueChanges
       .pipe(
-        // this is required otherwise an `ExpressionChangedAfterItHasBeenCheckedError` will happen
-        // this is due to the fact that parent component will define a given state for the form that might
-        // be changed once the children are being initialized
-        delay(0),
+        // note: we do not want to use startWith here
+        // because we've got to handle the first onChange alone
+        // into an async way (CF huge comment above) and without
+        // calling `onTouched` nor `onChange`
         tap(changes => {
           if (this.onTouched) {
             this.onTouched();