feat(ngModelOptions): allow to debounce 'default' only, and add catch…

[Narretz]

…-all

BREAKING CHANGE:

the 'default' key in 'debounce' now only debounces the default event, i.e. the event
that is added as an update trigger by the different input directives automatically.

Previously, it also applied to other update triggers defined in 'updateOn' that
did not have a corresponding key in the 'debounce'.

This behavior is now supported via a special wildcard / catch-all key: '*'.

See the following example:

Pre-1.7:
'mouseup' is debounced by 500 milliseconds because 'default' is applied.

ng-model-options="{
  updateOn: 'default blur mouseup',
  debounce: { 'default': 500, 'blur': 0 }
}

1.7:
'mouseup' is debounced by 1000 milliseconds because '*' is applied.

ng-model-options="{
  updateOn: 'default blur mouseup',
  debounce: { 'default': 500, 'blur': 0, '*': 1000 }
}

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature

What is the current behavior? (You can also link to an open issue here)
see #15411

What is the new behavior (if this is a feature change)?
see above

Does this PR introduce a breaking change?
YES, see above

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Fix/Feature: Docs have been added/updated
• Fix/Feature: Tests have been added; existing tests pass

Other information:

[petebacondarwin]

LGTM - I made a small suggestion about the description of the test.

In the commit message it would be helpful to spell out how one gets the old behaviour back if you don't like the breaking change.

[petebacondarwin]

'should allowing setting the debounce timeout for unspecified events'?

[gkalpak]

So, afaict all events that are not included in updateOn will get the default timeout.
Is this intended? It seems very counter-intuitive (and there don't seem to be tests and docs for that.)

_{(Sorry for joining the party late 😞)}

[petebacondarwin]

I guess this was partly to be in keeping with the previous behaviour, which meant that all items that were not listed the debounce property got the default value. Now only the items that are not listed in both the updateOn and the debounce property get the default value.
But given that we are going with the exciting new catch-all * then we should use that for things that are not specified in either place.

[Narretz]

I based this on the assumption that all events that are not in updateOn are the control's default events, i.e. the event handlers that the control defines itself which are never explicitly exposed via the API. For example, our inputs usually have the input (change for IE) as the default event, but this is not exposed on the ngModel controller, so I cannot match against it directly.
That also means if you add 'input' in 'onUpdate' it is not considered a 'default' event anymore, and doesn't receive debounce that you define in 'default'

[gkalpak]

Theoretically, trigger can be any event, because it is a public documented API of $setViewValue. The way this is implemented now is a bit weird (and probably broken). Basically, $setViewValue seems to assume that IF trigger is provided as an argument (it is optional), then it is on of the "default" event:

$setViewValue: function(value, trigger) {
  this.$viewValue = value;
  if (this.$options.getOption('updateOnDefault')) {
    this.$$debounceViewValueCommit(trigger);
  }
}

This holds true inside the framework (i.e. we only pass the trigger argument when calling $setViewValue from the input's "default" event. But there is nothing stopping people from passing their events.

I believe the current situation is unintuitive (and underdocumented). I consider it a bug: #14886.

Off the top of my head, the whole mechanism should be refactored in a way that allows:

1. Applying the appropriate debouncing for each event (even for non-default events passed by users).
2. Recognizing the default event(s) for each input/input-type.
3. Applying the default debouncing to default event(s).
4. Applying the catch-all (*) debouncing when trigger is not specified or not specifically mentioned in ngModelOptions#debounce (and not the default event).

WDYT?
(Note: It would be a breaking change, so if we agree on it should land it ASAP.)

[Narretz]

There's a difference between the default events / triggers und the updateOn events / triggers. Only the default triggers call setViewValue - the updateOn triggers call debounceViewValue directly (i.e. they require that the control calls setViewValue in the correct circumstances with the default event). So we assume that updateOn adds additional events to the default event, and that setViewValue is always called with the control's default event. Of course this is at the moment quite sketchy because we assume that the default event is input and that updateOn is blur, mouseup etc. (This whole distinction is something I haven't really thought about when writing this PR. I assumed the updateOn events called setViewValue).

But there is nothing stopping people from passing their events.

That is true, but imo this is not an intended use case. Every control defines its own setViewValue triggers (default triggers) - most are DOM events, but they don't have to. Calling a control's ngModelController.$setViewValue with a different trigger, i.e. programatically, is supported, but why would you do that? I can't think of a scenario where it would be necessary. If you need to update the model programatically, you do it from the scope. $setViewValue should only be called with the triggers the controls uses itself.

2. Recognizing the default event(s) for each input/input-type.

We can do this for our own inputs, but how would we handle custom controls that do not define their default events? I don't think many custom controls will be updated to define their default controls.
As explained above, I'd also argue this contradicts "1. Applying the appropriate debouncing for each event, (even for non-default events passed by users)." because if a control has well defined default events, why should you be able to set the viewValue with a different trigger?
If we have well defined default events, then we should also check if there's event duplication in updateOn - for example, in this plnkr, we call $$debounceViewValueCommit twice: http://plnkr.co/edit/0niAourDe8t2yknzzNX7?p=preview

4. Applying the catch-all (*) debouncing when trigger is not specified or not specifically mentioned in ngModelOptions#debounce (and not the default event).

Ok. But * would apply to default events if default isn't applied in debounce, right?

[gkalpak]

I probably haven't thought it through. It's just that default is confusing. Maybe it is the fact that default in updateOn has a different meaning than in debounce. And I also don't understand the reasoning behind this 😕:

  $setViewValue: function(value, trigger) {
    this.$viewValue = value;
    if (this.$options.getOption('updateOnDefault')) {
      this.$$debounceViewValueCommit(trigger);
    }
  }

TBH, I don't have a good suggestion of how this API should be (without breaking it completely 😁).

[Narretz]

Does 'default' really have a different meaning in updateOn and debounce? I don't think so, maybe you can elaborate.

What is the default event? The current docs say it's an event "that matches the default events belonging to the control." And imo this can only be the events that, when fired, call $setViewValue, because this is the glue between the control and ngModel. Then, if updateOnDefault is not set, setViewValue does not have to call debounceViewValue, because it should not update the modelValue.

Basically, that means you should only call setViewValue if you actually update the $viewValue. Imo, it doesn't make sense to have something like setViewValue(value, 'blur'), because in what circumstance can the blur event actually update the value?

[gkalpak]

AFAICT (by looking at the code - especially before thi PR), updteOn: default seemed to (mostly) refer to the default event for the component, while ``debounce.default` seemed to refer to a default debounce; i.e. a value to be used for an event type for which no value is explicitly set.

My assumption was that we would change that so that debounce.* would now mean the "default"/"fallback" debounce (for any event for which no explicit debounce is set) and debounce.default would refer to the debounce for the default event.

Then, if updateOnDefault is not set, setViewValue does not have to call debounceViewValue, because it should not update the modelValue.

That is unexpected (to me). I would expect it to call debounceViewValueCommit for any event that is included in updateOn (in addition to the default event).

[Narretz]

Ok, I see. I think that now the behavior is more consistent, as "default" refers to the same thing in updateOn and default.

The setViewValue behavior is technically independant from this change - we could change it at a later point, but personally I think it's fine as it is.