Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Merged
merged 1 commit into from
Nov 21, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/ng/directive/ngModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,8 +838,12 @@ NgModelController.prototype = {

if (isNumber(debounceDelay[trigger])) {
debounceDelay = debounceDelay[trigger];
} else if (isNumber(debounceDelay['default'])) {
} else if (isNumber(debounceDelay['default']) &&
this.$options.getOption('updateOn').indexOf(trigger) === -1
Copy link
Member

Choose a reason for hiding this comment

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

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 😞)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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'

Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@Narretz Narretz Nov 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

  1. 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

  1. 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?

Copy link
Member

Choose a reason for hiding this comment

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

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 😁).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

) {
debounceDelay = debounceDelay['default'];
} else if (isNumber(debounceDelay['*'])) {
debounceDelay = debounceDelay['*'];
}

this.$$timeout.cancel(this.$$pendingDebounce);
Expand Down
8 changes: 8 additions & 0 deletions src/ng/directive/ngModelOptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,14 @@ defaultModelOptions = new ModelOptions({
* debounce: { 'default': 500, 'blur': 0 }
* }"
* ```
* You can use the `*` key to specify a debounce value that applies to all events that are not
* specifically listed. In the following example, `mouseup` would have a debounce delay of 1000:
* ```
* ng-model-options="{
* updateOn: 'default blur mouseup',
* debounce: { 'default': 500, 'blur': 0, '*': 1000 }
* }"
* ```
* - `allowInvalid`: boolean value which indicates that the model can be set with values that did
* not validate correctly instead of the default behavior of setting the model to undefined.
* - `getterSetter`: boolean value which determines whether or not to treat functions bound to
Expand Down
36 changes: 34 additions & 2 deletions test/ng/directive/ngModelOptionsSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,9 +498,41 @@ describe('ngModelOptions', function() {

helper.changeInputValueTo('c');
browserTrigger(helper.inputElm, 'mouseup');
// counter-intuitively `default` in `debounce` is a catch-all
// `default` in `debounce` only affects the event triggers that are not defined in updateOn
expect($rootScope.name).toEqual('c');
});


it('should use the value of * to debounce all unspecified events',
function() {
var inputElm = helper.compileInput(
'<input type="text" ng-model="name" name="alias" ' +
'ng-model-options="{' +
'updateOn: \'default blur mouseup\', ' +
'debounce: { default: 10000, blur: 5000, \'*\': 15000 }' +
'}"' +
'/>');

helper.changeInputValueTo('a');
expect($rootScope.name).toBeUndefined();
$timeout.flush(6000);
expect($rootScope.name).toBeUndefined();
$timeout.flush(4000);
expect($rootScope.name).toEqual('a');

helper.changeInputValueTo('b');
browserTrigger(inputElm, 'blur');
$timeout.flush(4000);
expect($rootScope.name).toEqual('a');
$timeout.flush(2000);
expect($rootScope.name).toEqual('b');
$timeout.flush(10000);

helper.changeInputValueTo('c');
browserTrigger(helper.inputElm, 'mouseup');
expect($rootScope.name).toEqual('b');
$timeout.flush(10000); // flush default
expect($rootScope.name).toEqual('b');
$timeout.flush(5000);
expect($rootScope.name).toEqual('c');
});

Expand Down