Custom change detection with onPush is broken

[maxime1992]

Related to Angular issues with ControlValueAccessor and OnPush components:

• NgModel works incorrectly when using onPush ChangeDetection strategy angular/angular#10816
• ExpressionChangedAfterItHasBeenCheckedError with reactive form and ngClass relying on input.value angular/angular#25191
• ExpressionChangedAfterItHasBeenCheckedError is thrown by ngModel.invalid above the ngModel control declaration angular/angular#18748
• ChangeDetectionStrategy.OnPush breaks ControlValueAccessor functionality angular/angular#12607
• Forms: NgControlStatus directive won't update if in ChangeDetectionStrategy.OnPush angular/angular#11563

Currently, setting a component to use the OnPush change detection strategy is not safe with ngx-sub-form as you'd end up with a "shift". If you type "Hello" in an input and then add "A" the displayed value would be "Hello". If after that you add a B, the value displayed would be "HelloA", etc. Late by 1 change basically.

When calling the onChange hook from the ControlValueAccessor, Angular should run a change detection but it seems that it's not the case.

Workaround

Not a beautiful one but at least simple and it's still possible to use OnPush...

In order to trigger a change detection, we can use the async pipe with a value coming from the form. So for every form that has at least a child (otherwise it's not needed), you can do the following: [attr.data-ngx-sub-form-issue-93]="formGroup.valueChanges | async".

For e.g., from a sub component:

<div [formGroup]="formGroup" [attr.data-ngx-sub-form-issue-93]="formGroup.valueChanges | async">
  <-- ... -->
</div>

[maxime1992]

@zakhenry we should list our options here before working on a PR :)
Feel free to add other ideas but here's what we thought of previously:

Ideas	Pros	Cons
Require to pass the ChangeDetectorRef when creating a root or sub form	Easy to implement Easy to understand what's going on	Boilerplate for every component If every library was doing that it'd be a huge pain
Use a custom directive to access the ChangeDetectorRef	Minimal boilerplate	Easy to forget
Use a directive on [formGroup] to access the ChangeDetectorRef	No further boilerplate required	Will only work for reactive forms... So not really an option

[maxime1992]

With Angular v9 around the corner and Ivy, we might be able to do all that without having to create a module and pass the ChangeDetectorRef 🙌.

We should be able to mark the component as dirty by passing the component's reference (this) 🔥

So we should put that on hold as not having to setup a module is quite nice and if we can keep it that way we should probably

[maxime1992]

We've made a PR recently at work where we also had to apply the fix with the status otherwise a disabled button was not picked up so instead of

<div [formGroup]="formGroup" [attr.data-ngx-sub-form-issue-93]="formGroup.valueChanges | async">
  <-- ... -->
</div>

We had to do

<div
  [formGroup]="formGroup"
  [attr.data-ngx-sub-form-issue-93-1]="formGroup.valueChanges | async"
  [attr.data-ngx-sub-form-issue-93-2]="formGroup.statusChanges | async"
>
  <-- ... -->
</div>

Which starts to be really painful especially when repeating that on multiple forms 😅

So we came up with a directive that lets us do:

<div [ngxSubForm]="formGroup">
  <-- ... -->
</div>

The directive is the following:

@Directive({
  selector: '[ngxSubForm]',
  providers: [{ provide: ControlContainer, useExisting: forwardRef(() => NgxSubFormDirective) }],
})
export class NgxSubFormDirective extends FormGroupDirective implements OnInit, OnDestroy {
  // following line needs the disable as the type is defined in FormGroupDirective
  // and overriding it throws an error
  // tslint:disable-next-line: no-non-null-assertion
  @Input('cncNgxSubForm') public form: FormGroup = null!;

  constructor(
    @Optional() @Self() @Inject(NG_VALIDATORS) validators: any[],
    @Optional() @Self() @Inject(NG_ASYNC_VALIDATORS) asyncValidators: any[],
    private cd: ChangeDetectorRef,
  ) {
    super(validators, asyncValidators);
  }
  public ngOnInit(): void {
    if (!this.valueChanges || !this.statusChanges) {
      throw new Error('valueChanges and/or statusChanges not defined');
    }
    merge(this.valueChanges, this.statusChanges)
      .pipe(
        tap(() => this.cd.markForCheck()),
        takeUntilDestroyed(this),
      )
      .subscribe();
  }
  public ngOnDestroy(): void {}
}

I'd be glad to publish this into our package so everyone can just use it but remember, ngx-sub-form is just built with classes that you extends. We do not have any angular module 🙌 (for now, that day may come). It'd feel wrong to introduce a module for a temporary bug fix as, as stated in the previous comment, Ivy might let us run a CD cycle from a function call directly which would make that directive useless.

In the meantime, I'd say use the quick workaround <div [formGroup]="formGroup" [attr.data-ngx-sub-form-issue-93]="formGroup.valueChanges | async"> if you only need it a few times otherwise copy the code of the directive above if you start doing that repeatidly 👍

We're aware it's not ideal but will start dealing with that once Ivy let us go around it 😸

[maxime1992]

Small update, here's the directive we use:

@Directive({
  selector: '[ngxSubFormChangeDetectorFix]',
})
export class NgxSubFormChangeDetectorFixDirective implements AfterViewInit, OnDestroy {
  constructor(private zone: NgZone, private cd: ChangeDetectorRef, private formGroup: FormGroupDirective) {}

  public ngAfterViewInit(): void {
    if (!this.formGroup.valueChanges || !this.formGroup.statusChanges) {
      throw new Error('valueChanges or statusChanges not defined');
    }

    // see https://github.com/cloudnc/ngx-sub-form/issues/143
    // once resolved, this shouldn't be needed anymore
    const formGroupDisabled$: Observable<any> = new Observable(observer => {
      this.zone.runOutsideAngular(() => {
        const subscription = interval(50)
          .pipe(tap(() => observer.next(this.formGroup.disabled)))
          .subscribe();

        return () => subscription.unsubscribe();
      });
    }).pipe(distinctUntilChanged());

    merge(this.formGroup.valueChanges, this.formGroup.statusChanges, formGroupDisabled$)
      .pipe(
        // https://gitmemory.com/issue/angular/angular/29606/564893047
        // https://github.com/angular/angular/issues/17013
        delay(0),
        tap(() => this.cd.detectChanges()),
        takeUntilDestroyed(this),
      )
      .subscribe();
  }

  public ngOnDestroy(): void {}
}

⚠️ It is really dirty and there's nothing to be proud of here :( ⚠️

But this is a temporary hack until we can come up with a better solution.

Now that Angular 9 has been released for quite a while we could give a go to the function to run the CD directly (just like it's done by ngrx/components package).

[maxime1992]

🎉 This issue has been resolved in version 5.2.0-feat-rewrite.1 🎉

The release is available on:

• npm package (@feat-rewrite dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[maxime1992]

Alright after trying it previsouly then reverting the fix, I finally gave this another go today.

I've added OnPush for all the components in the demo app using the rewrite. This broke a few e2e tests so I had something to iterate with quickly.

I believe that by using markDirty it is now fixed. Here's the commit: 274420a

All the e2e tests are passing correctly again

[github-actions]

🎉 This issue has been resolved in version 5.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[maxime1992]

Hey good news! We may not need markDirty anymore 🤔

angular/angular#10816 (comment)