ngModelOptions debounce was failing when on event with a value of 0 #7205
Conversation
|
The patch looks good, but can you please fix the jshint error. Thanks. |
|
@lgalfaso Thank you for the comments, I've addressed and pushed them. I didn't understand the "not needed" via the email :> |
|
@joelhooks The "not needed" was part of the review, the else case was not needed as the initialization would take care of it. One way or another please
If everything goes as normal, this should be part of the next beta |
…alue of 0
Was testing ngModelOptions, and noticed that the provided example
`ngModelOptions="{ updateOn: 'default blur', debounce: {'default': 500, 'blur':
0} }"` didn't function. With a little investigating I determined that the logic
operation was skipping debounce[trigger] if it was 0 (thanks JS falsiness!).
My first solution was to do this:
```javascript
var debounceDelay = ctrl.$options && (
isObject(ctrl.$options.debounce)
? (isNumber(ctrl.$options.debounce[trigger]) ?
ctrl.$options.debounce[trigger]
:
(ctrl.$options.debounce['default']
|| 0))
: ctrl.$options.debounce
) || 0;
```
Which hopefully makes us all cringe. Instead of adding this mess (the original
logic took me about 30 minutes to understand), I decided to unroll it into
something a bit less... terse.
I added a unit test to cover this scenario.
|
@lgalfaso Commits have been squashed and the CLA signed. 👍 |
|
@joelhooks did you sign the CLA with the same email that you used to commit this PR? |
|
I did, but I did so again for good measure. |
|
OK, I am just waiting for Mary Poppins to confirm. Thanks! |
|
Oops! I just noticed this PR. Sorry for introducing the bug :( |
|
@IgorMinar – the CLA script seems to not be running :( |
Request Type: bug
How to reproduce: Here's a fiddle that demonstrates.
Component(s): misc core
Impact: medium
Complexity: small
This issue is related to:
Detailed Description:
Was testing ngModelOptions, and noticed that the provided example
ngModelOptions="{ updateOn: 'default blur', debounce: {'default': 500, 'blur': 0} }"didn't function. With a little investigating I determined that the logicoperation was skipping debounce[trigger] if it was 0 (thanks JS falsiness!).
My first solution was to do this:
Which hopefully makes us all cringe. Instead of adding this mess (the original
logic took me about 30 minutes to understand), I decided to unroll it into
something a bit less... terse.
I added a unit test to cover this scenario.
This was found in 1.3.0-beta.6.
Here's a fiddle that demonstrates. Notice that it debounces blur, despite it being configured with 0. THis is because
(ctrl.$options.debounce[trigger] || ctrl.$options.debounce['default'] || 0)will always skip to default if it exists anddebounce[trigger]is 0. Without a default alongside the trigger, the final 0 would be the result, and I suspect that is how this slipped by.Other Comments: