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

perf(input): prevent multiple validations on compilation #16760

Merged
merged 7 commits into from
Dec 5, 2018
+594 −68
Merged

perf(input): prevent multiple validations on compilation #16760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c1bef69
perf(input): prevent multiple validations on compilation
Narretz Oct 29, 2018
0d826b8
fixup! perf(input): prevent multiple validations on compilation
Narretz Nov 19, 2018
f2e1bd6
fixup! perf(input): prevent multiple validations on compilation
Narretz Nov 20, 2018
b4d7b61
fixup! perf(input): prevent multiple validations on compilation
Narretz Nov 21, 2018
b069361
fixup! perf(input): prevent multiple validations on compilation
Narretz Nov 23, 2018
57b45e1
fixup! perf(input): prevent multiple validations on compilation
Narretz Nov 29, 2018
4d82020
fixup! perf(input): prevent multiple validations on compilation
Narretz Dec 3, 2018
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
98 changes: 69 additions & 29 deletions src/ng/directive/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1497,7 +1497,7 @@ function createDateParser(regexp, mapping) {
}

function createDateInputType(type, regexp, parseDate, format) {
return function dynamicDateInputType(scope, element, attr, ctrl, $sniffer, $browser, $filter) {
return function dynamicDateInputType(scope, element, attr, ctrl, $sniffer, $browser, $filter, $parse) {
badInputChecker(scope, element, attr, ctrl, type);
baseInputType(scope, element, attr, ctrl, $sniffer, $browser);

Expand Down Expand Up @@ -1540,24 +1540,34 @@ function createDateInputType(type, regexp, parseDate, format) {
});

if (isDefined(attr.min) || attr.ngMin) {
var minVal;
var minVal = attr.min || $parse(attr.ngMin)(scope);
var parsedMinVal = parseObservedDateValue(minVal);

ctrl.$validators.min = function(value) {
return !isValidDate(value) || isUndefined(minVal) || parseDate(value) >= minVal;
return !isValidDate(value) || isUndefined(parsedMinVal) || parseDate(value) >= parsedMinVal;
};
attr.$observe('min', function(val) {
minVal = parseObservedDateValue(val);
ctrl.$validate();
if (val !== minVal) {
parsedMinVal = parseObservedDateValue(val);
minVal = val;
ctrl.$validate();
}
});
}

if (isDefined(attr.max) || attr.ngMax) {
var maxVal;
var maxVal = attr.max || $parse(attr.ngMax)(scope);
var parsedMaxVal = parseObservedDateValue(maxVal);

ctrl.$validators.max = function(value) {
return !isValidDate(value) || isUndefined(maxVal) || parseDate(value) <= maxVal;
return !isValidDate(value) || isUndefined(parsedMaxVal) || parseDate(value) <= parsedMaxVal;
};
attr.$observe('max', function(val) {
maxVal = parseObservedDateValue(val);
ctrl.$validate();
if (val !== maxVal) {
parsedMaxVal = parseObservedDateValue(val);
maxVal = val;
ctrl.$validate();
}
});
}

Expand Down Expand Up @@ -1709,50 +1719,68 @@ function isValidForStep(viewValue, stepBase, step) {
return (value - stepBase) % step === 0;
}

function numberInputType(scope, element, attr, ctrl, $sniffer, $browser) {
function numberInputType(scope, element, attr, ctrl, $sniffer, $browser, $filter, $parse) {
badInputChecker(scope, element, attr, ctrl, 'number');
numberFormatterParser(ctrl);
baseInputType(scope, element, attr, ctrl, $sniffer, $browser);

var minVal;
var maxVal;
var parsedMinVal;

if (isDefined(attr.min) || attr.ngMin) {
var minVal = attr.min || $parse(attr.ngMin)(scope);
parsedMinVal = parseNumberAttrVal(minVal);

ctrl.$validators.min = function(modelValue, viewValue) {
return ctrl.$isEmpty(viewValue) || isUndefined(minVal) || viewValue >= minVal;
return ctrl.$isEmpty(viewValue) || isUndefined(parsedMinVal) || viewValue >= parsedMinVal;
};

attr.$observe('min', function(val) {
minVal = parseNumberAttrVal(val);
// TODO(matsko): implement validateLater to reduce number of validations
ctrl.$validate();
if (val !== minVal) {
parsedMinVal = parseNumberAttrVal(val);
minVal = val;
// TODO(matsko): implement validateLater to reduce number of validations
ctrl.$validate();
}
});
}

if (isDefined(attr.max) || attr.ngMax) {
var maxVal = attr.max || $parse(attr.ngMax)(scope);
var parsedMaxVal = parseNumberAttrVal(maxVal);

ctrl.$validators.max = function(modelValue, viewValue) {
return ctrl.$isEmpty(viewValue) || isUndefined(maxVal) || viewValue <= maxVal;
return ctrl.$isEmpty(viewValue) || isUndefined(parsedMaxVal) || viewValue <= parsedMaxVal;
};

attr.$observe('max', function(val) {
maxVal = parseNumberAttrVal(val);
// TODO(matsko): implement validateLater to reduce number of validations
ctrl.$validate();
if (val !== maxVal) {
parsedMaxVal = parseNumberAttrVal(val);
maxVal = val;
// TODO(matsko): implement validateLater to reduce number of validations
ctrl.$validate();
}
});
}

if (isDefined(attr.step) || attr.ngStep) {
var stepVal;
var stepVal = attr.step || $parse(attr.ngStep)(scope);
var parsedStepVal = parseNumberAttrVal(stepVal);

ctrl.$validators.step = function(modelValue, viewValue) {
return ctrl.$isEmpty(viewValue) || isUndefined(stepVal) ||
isValidForStep(viewValue, minVal || 0, stepVal);
return ctrl.$isEmpty(viewValue) || isUndefined(parsedStepVal) ||
isValidForStep(viewValue, parsedMinVal || 0, parsedStepVal);
};

attr.$observe('step', function(val) {
stepVal = parseNumberAttrVal(val);
// TODO(matsko): implement validateLater to reduce number of validations
Copy link
Member

Choose a reason for hiding this comment

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

Move to before ctrl.$validate() for consistency?

ctrl.$validate();
if (val !== stepVal) {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that we should be also checking for minVal here, because the step validation depends on that.

Copy link
Contributor Author

@Narretz Narretz Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You mean ensure that the minVal has changed? That doesn't work, because we need to run this also if the minVal is the same.
This is a good point though - the changes in this PR only prevent multiple calls to $validate immediately after compilation / linking. If you have a range or number with more than one modifier, i.e. step, max, min and change all values in a single digest, every observer willcall $validate.
I don't think there's an easy way to prevent this, because the observers are independent of each other. Maybe that's what Matias meant with validateLater - a way to collect the $validate calls of a single "validation group" (For the record, I thought he meant a function that collects all $validate calls on an element).
Long story short, I don't think we can do anything about this without making bigger changes. I can adjust the commit message to reflect that changing multiple observed values will still result in multiple validate calls. (I believe this happens much rarer though, so the PR is still worthwhile).

Copy link
Member

@gkalpak gkalpak Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No, I don't meant ensure that minVal has changed. What I meant is that that validator needs to run if either of minVal or stepVal has changed. I.e. even if stepVal is the same, but minVal has changed.

I am pretty sure that indeed Matias meant avoid multiple calls to $validate() 😁
We could easily do this by running $validate() asynchronously (e.g. adding it in the $postDigest queue and only if it is not already scheduled), but that would be a breaking change (which does not make sense at this point).

We could use a similar technique in the validators as this PR uses in observers:
Keep track of the involved values (e.g. module/viewValue, validators params, etc) and do not re-run validation unless the values have changed. But that would complicate things even further, open up new possibilities for memory leaks (if not handled carefully) and would also not help with the stateful async validators (which are usually the most expensive ones). So, again, not worth it atm.

I believe this happens much rarer though, so the PR is still worthwhile.

Actually, I believe it will happen all the time 😛
Basically, afaict, saves some (half) of the re-runs during initialization, but does nothing to prevent multiple runs as the user interacts with the input (which is where the majority of runs comes from throughout the lifetime of an input).

If we can keep it clean and BC-free (with reasonable confidence 😁), then it is definitely worth landing. But I wouldn't go to great extends, complicating the internal logic too much etc., just to save a few runs 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't me ensure that minVal has changed. What I meant is that that validator needs to run if either of minVal or stepVal has changed. I.e. even if stepVal is the same, but minVal has changed.

Right, but if the minVal has changed, it will call $validate, which will also run the step validator. Since we are not fixing multiple runs after initialization , we don't have to deal with this,

Actually, I believe it will happen all the time

It will only call $validate multiple times if observed values change, which I think is much rarer than initialization, which happens always. ;)
If you change the input value, only one call to $validate from inside ngModel will be made.
The issue that this addresses (#14691) is also about load / initialization, not interaction primarily.
Initially I also thought about using postDigest, but as you said, it's too late for that now.

parsedStepVal = parseNumberAttrVal(val);
stepVal = val;
ctrl.$validate();
}

});

}
}

Expand Down Expand Up @@ -1782,6 +1810,8 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) {
originalRender;

if (hasMinAttr) {
minVal = parseNumberAttrVal(attr.min);

ctrl.$validators.min = supportsRange ?
// Since all browsers set the input to a valid value, we don't need to check validity
function noopMinValidator() { return true; } :
Expand All @@ -1794,6 +1824,8 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) {
}

if (hasMaxAttr) {
maxVal = parseNumberAttrVal(attr.max);

ctrl.$validators.max = supportsRange ?
// Since all browsers set the input to a valid value, we don't need to check validity
function noopMaxValidator() { return true; } :
Expand All @@ -1806,6 +1838,8 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) {
}

if (hasStepAttr) {
stepVal = parseNumberAttrVal(attr.step);

ctrl.$validators.step = supportsRange ?
function nativeStepValidator() {
// Currently, only FF implements the spec on step change correctly (i.e. adjusting the
Expand All @@ -1827,7 +1861,13 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) {
// attribute value when the input is first rendered, so that the browser can adjust the
// input value based on the min/max value
element.attr(htmlAttrName, attr[htmlAttrName]);
attr.$observe(htmlAttrName, changeFn);
var oldVal = attr[htmlAttrName];
attr.$observe(htmlAttrName, function wrappedObserver(val) {
if (val !== oldVal) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above: I believe minVal should be checked along with stepVal.

oldVal = val;
changeFn(val);
}
});
}

function minChange(val) {
Expand Down Expand Up @@ -1881,11 +1921,11 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) {
}

// Some browsers don't adjust the input value correctly, but set the stepMismatch error
if (supportsRange && ctrl.$viewValue !== element.val()) {
ctrl.$setViewValue(element.val());
} else {
if (!supportsRange) {
// TODO(matsko): implement validateLater to reduce number of validations
ctrl.$validate();
} else if (ctrl.$viewValue !== element.val()) {
ctrl.$setViewValue(element.val());
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/ng/directive/ngModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,7 @@ NgModelController.prototype = {
* `$modelValue`, i.e. either the last parsed value or the last value set from the scope.
*/
$validate: function() {

// ignore $validate before model is initialized
if (isNumberNaN(this.$modelValue)) {
return;
Expand Down
Loading