webcat12345 / ngx-ui-switch

Angular UI Switch component
https://webcat12345.github.io/ngx-ui-switch/demo/
MIT License
111 stars 68 forks source link

ReactiveForms - patchValue and setValue does not work correctly with option emitEvent false #493

Open blazekv opened 10 months ago

blazekv commented 10 months ago

If ui-switch is part of reactive forms and there is set or patch method used with option emit event false it does not work.

Example code for demo:

demo.component.ts

  constructor(private fb: NonNullableFormBuilder) {
    this.form = this.fb.group({
      test: [false]
    })
    this.form.valueChanges.subscribe((value) => console.log('Changed', value));
  }

    test() {
       this.form.patchValue({test: false}, {emitEvent: false})
    }

demo.component.html

    <div [formGroup]="form">
      <ui-switch formControlName="test" checked [ariaLabel]="'checked'"></ui-switch>
    </div>
    <button (click)="test()">change</button>

Expected behaviour: If button change is clicked there is no output in console (because of emitEvent is set to false)

Actual behaviour: If button change is clicked there is "Changed" output in console (emitEvent option does not work)

Reason of this strange behaviour is because onChange is called while setting value (in writeValue method), but it was added because some kind of bug - https://github.com/webcat12345/ngx-ui-switch/commit/1382bb140422c5efca4df9d6e15030da3535c2e2

jeandat commented 5 months ago

Took me a while to find out it came from ui-switch. I confirm, emitEvent is not respected.

jeandat commented 5 months ago

Here is a minimalistic repo to reproduce.

jeandat commented 5 months ago

Indeed in code there is this:

writeValue(obj: any): void {
    if (obj !== this.checked) {
      this.checked = !!obj;
    }

    this.onChangeCallback(this.checked); // <---- HERE
    if (this.cdr) {
      this.cdr.markForCheck();
    }
  }

Why would you want to call the CVA callback when you receive a new value? You should call it only when the internal value has changed after a user action.

cmckni3 commented 1 week ago

Research

blazekv commented 1 week ago

@cmckni3 These links means that you think that implementation is correct? Because I am not sure about that. Just look at the documentation

https://angular.dev/api/forms/ControlValueAccessor

writeValue is API for informing your ControlValueAccessor that forms API changed value and onChange callback should be used to inform API that something has changed inside your component. But if you used it together it will break that behaviour. Because event that something has changed is emitted from component and it does not have flag emitEvent. So it is correct that this is emitted. But if you bind it together the result is that by calling writeValue you will also call change and that event is emitted despite flag set to false.

Angular valueChanges is broken that is for sure. But in this case it works as intended. I dont think that this.onChangeCallback(this.checked); should be in the writeValue at all.

cmckni3 commented 1 week ago

@cmckni3 These links means that you think that implementation is correct? Because I am not sure about that. Just look at the documentation

@blazekv No, not at all. Yes, I know the code has an issue, and #493 is a valid bug. I am just getting some time to look into it again. I dumped the links above for reference from when I was researching in January.

writeValue is API for informing your ControlValueAccessor that forms API changed value and onChange callback should be used to inform API that something has changed inside your component. But if you used it together it will break that behaviour. Because event that something has changed is emitted from component and it does not have flag emitEvent. So it is correct that this is emitted. But if you bind it together the result is that by calling writeValue you will also call change and that event is emitted despite flag set to false.

Yes, I know.

Angular valueChanges is broken that is for sure. But in this case it works as intended. I dont think that this.onChangeCallback(this.checked); should be in the writeValue at all.

Agreed. I am working on tracking down why the change was added in 1382bb140422c5efca4df9d6e15030da3535c2e2. I think it was due to a bug in template driven forms, but should not have been changed.

I am working on validation (testing) right now and should have a PR to release soon.


https://angular.dev/api/forms/ControlValueAccessor

Neat, the Angular docs look a lot better visually and contain more information.