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

Perhaps better way to update ngrx on form.valuechanges() #527

Closed
BBX999 opened this issue May 16, 2020 · 2 comments · Fixed by #559
Closed

Perhaps better way to update ngrx on form.valuechanges() #527

BBX999 opened this issue May 16, 2020 · 2 comments · Fixed by #559

Comments

@BBX999
Copy link

BBX999 commented May 16, 2020

I noticed in the current form.component.html the following code is used in the template to update the ngrx model:

<ng-container *ngIf="formValueChanges$ | async as updatedForm">
  {{update(updatedForm)}}
</ng-container>

I found that this triggers multiple and duplicate actions for even a single key entry on an input field. It also triggers the action on clicks within the form when data has not changed. I am wondering if this is by design or whether the duplicate actions are tolerated for the sake of the automatic unsubscribe by using the | async pipe.

If the duplication is tolerated for the sake of the automatic unsubscribe, I think I have a possible improvement. Instead of a function call within the template, the template would just contain the subscription:

<ng-container *ngIf="formValueChanges$ | async as updatedForm">
</ng-container>

but the component would provide for the action to be triggered in a tap operator of the formValueChanges$ observable:

ngOnInit() {
    this.formValueChanges$ = this.form.valueChanges.pipe(
      debounceTime(500),
      filter((form: Form) => form.autosave),
      tap(updatedForm => update(updatedForm))
    );
}

This seems to avoid duplicate actions. Thoughts? Any potential drawbacks to this solution?

@millbj92
Copy link
Contributor

millbj92 commented Aug 8, 2021

Submitted a PR for this, btw. Nice catch man!

@BBX999
Copy link
Author

BBX999 commented Aug 30, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants