Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nz-code-editor's ControlValueAccessor triggers superfluous view→model updates #5869

Closed
Airblader opened this issue Oct 6, 2020 · 4 comments · Fixed by #5933
Closed

nz-code-editor's ControlValueAccessor triggers superfluous view→model updates #5869

Airblader opened this issue Oct 6, 2020 · 4 comments · Fixed by #5933

Comments

@Airblader
Copy link
Collaborator

Airblader commented Oct 6, 2020

Steps to reproduce

The ControlValueAccessor for nz-code-editor seems to have an issue where a model->view update will also trigger a view->model update, which can cause all kinds of issues like infinite loops when subscribing to form changes and then writing back into the control.

Environment Info
ng-zorro-antd 9.1.2
Browser Chrome
@Airblader Airblader changed the title nz-code-editor's form implementation is broken nz-code-editor's ControlValueAccessor triggers superfluous view→model updates Oct 7, 2020
@Airblader
Copy link
Collaborator Author

FYI I couldn't provide a Stackblitz because I can't get nz-code-editor to work in there.

@wzhudev
Copy link
Member

wzhudev commented Oct 10, 2020

Fixed by #5872. Thanks for your issue and PR! 🥂

@wzhudev wzhudev closed this as completed Oct 10, 2020
@Airblader
Copy link
Collaborator Author

@wendellhu95 My PR was a different issue, this issue is not fixed by that PR.

@vthinkxie vthinkxie reopened this Oct 10, 2020
@Airblader
Copy link
Collaborator Author

Airblader commented Oct 10, 2020

I can't open a PR at the moment, but if someone wants to send the fix for this, I believe all that has to be done is change emitValue in code-editor.component.ts to this (but untested, so please double check):

  private emitValue(value: string): void {
    if (this.value === value) {
      // If the value didn't change there's no reason to send an update.
      // Specifically this may happen during an update from the model (writeValue) where sending an update to the model would actually be incorrect.
      return;
    }

    this.value = value;
    this.onChange(value);
  }

If you want to test this, essentially we need to make sure that calling writeValue does not trigger the onChange function.

Airblader added a commit to Airblader/ng-zorro-antd that referenced this issue Oct 16, 2020
Before notifying Angular (through the ControlValueAccessor mechanism) of a change to
the value we make sure that the value has actually changed. This is necessary to
avoid feedback loops as writeValue (model -> view) would otherwise cause this to be
fired (view -> model) which can cause infinite loop issues on the usage side or
broken validation detection.

fixes NG-ZORRO#5869
vthinkxie pushed a commit that referenced this issue Oct 16, 2020
Before notifying Angular (through the ControlValueAccessor mechanism) of a change to
the value we make sure that the value has actually changed. This is necessary to
avoid feedback loops as writeValue (model -> view) would otherwise cause this to be
fired (view -> model) which can cause infinite loop issues on the usage side or
broken validation detection.

fixes #5869
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants