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

fix(form-plugin): introduce conditional debounce #1061

Merged
merged 4 commits into from
May 17, 2019
Merged

fix(form-plugin): introduce conditional debounce #1061

merged 4 commits into from
May 17, 2019

Conversation

FortinFred
Copy link
Contributor

@FortinFred FortinFred commented May 16, 2019

make the form-plugin dispatch actions immediately when the form's updateOn is 'blur' or 'submit' or when the ngxsFormDebounce is less than zero

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

#365

Issue Number: 365

What is the new behavior?

When a ngxsFormDebounce is less than zero or when a FormGroup's updateOn property is configured with 'blur' or 'submit', the debounceTime operator will be remove from the valueChanges and statusChanges pipes.

It seems that even with a timer of zero, the state is updated asynchronously after the form has been submited. Removing the operator in the describe situation resolve the issue.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

make the form-plugin dispath actions immediately when the form's updateOn is 'blur' or 'submit' or when the ngxsFormDebounce is less than zero
@splincode
Copy link
Member

Please add tests to your new functionality.

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks for the contribution.
@FortinFred Do you think there could be any scenario where an application would rely on the broken behavior?
cc @splincode @arturovt

@FortinFred
Copy link
Contributor Author

@markwhitfeld
No, I can't really see any scenario where this would be disarable.

@FortinFred
Copy link
Contributor Author

FortinFred commented May 17, 2019

@markwhitfeld
would also add that I understand the reason for the debounce mechanism to exist on the ngxsForm but such feature IMO would better be served by Angular itself.
angular/angular#19705

If the linked FR ever gets implemented, the ngxsFormDebounce would be deprecated and actions would be dispatched synchronously,

@markwhitfeld
Copy link
Member

@FortinFred Do you think we need any updates to the docs around this?

Copy link
Member

@arturovt arturovt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@markwhitfeld markwhitfeld added this to the 3.5.0 milestone May 17, 2019
@FortinFred
Copy link
Contributor Author

@markwhitfeld
I think it might make sense mentionning that the ngxsDebouce is ignored when the form's onUpdate === 'blur | submit'

Frédéric Fortin and others added 2 commits May 17, 2019 16:21
add a statement about ngxsFormDebounce being ignored for forms with updateOn = 'blur | submit'
@splincode
Copy link
Member

Good job!

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

Successfully merging this pull request may close these issues.

4 participants