fix(ngModel, input): improve handling of built-in named parsers #16347
Conversation
There was a problem hiding this comment.
Generally LGTM, except:
-
AFAICT, the breaking change can potentialy affect all input types, not just email, url, date/month/time/datetime-local/week, as mentioned in the commit message.
-
I don't think it properly addresses the problem of synonymous parsers and validators (see refactor(input): remove $$parserName from email, url #14292 (comment) for more details).
But overall, I think this is a nice change 💯
Hm, I don't see how. Only these input types have parsers that rely on the (private) $$parserName property. Sure, somebody could have ued it for their own parser, but this was not documented or supported, and mentioning it in the BC notice might confuse people.
The original original issue (#14249) was about duplicated class setting, that is right. However, this was fixed in ngAnimate. Now we could optimize this in ngModel before the classes are set, but I don't think this is really related to this issue .. or is it?? 😱 |
USers can add parsers to any input. Previously, a parse error on a number or range input, for example, would be reported as |
It is still unexpected that we may add/remove classes twice. Might not cause an issue with |
Ah, you are right. number and range also fall into this category.
Is it unexpected if it doesn't have any effect apart from a small perf penalty? We can track this in a new issue. |
This commit changes how input elements use the private $$parserName property on the ngModelController to name parse errors. Until now, the input types (number, date etc.) would set $$parserName when the inputs were initialized, which meant that any other parsers on the ngModelController would also be named after that type. The effect of that was that the `$error` property and the `ng-invalid-...` class would always be that of the built-in parser, even if the custom parser had nothing to do with it. The new behavior is that the $$parserName is only set if the built-in parser is invalid i.e. returns `undefined`. Also, $$parserName has been removed from input[email] and input[url], as these types do not have a built-in parser anymore. BREAKING CHANGE: *Custom* parsers that fail to parse on input types "email", "url", "number", "date", "month", "time", "datetime-local", "week", do no longer set `ngModelController.$error[inputType]`, and the `ng-invalid-[inputType]` class. Also, custom parsers on input type "range" do no longer set `ngModelController.$error.number` and the `ng-invalid-number` class. Instead, any custom parsers on these inputs set `ngModelController.$error.parse` and `ng-invalid-parse`. Closes angular#14292 Closes angular#10076
Narretz
force-pushed
the
fix-ngmodel-parser
branch
from
25586dc to
08f6f99
Compare
This commit changes how input elements use the private $$parserName
property on the ngModelController to name parse errors. Until now,
the input types (number, date etc.) would set $$parserName when
the inputs were initialized, which meant that any other parsers on
the ngModelController would also be named after that type. The
effect of that was that the
$errorproperty and theng-invalid-...class would always be that of the built-in parser, even if the custom
parser had nothing to do with it.
The new behavior is that the $$parserName is only set if the actual
parser is invalid, i.e. returns
undefined.Also, $$parserName has been removed from input[email] and input[url],
as these types do not have a built-in parser anymore.
BREAKING CHANGE:
Custom parsers that fail to parse on input types email, url, date, month, time,
datetime-local, week, do no longer set $error[inputType], and no longer set the class
ng-invalid-[inputType]. Instead, they set $error.parse andng-invalid-parse.Closes #14292
Closes #10076
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bugfix
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Yes
Please check if the PR fulfills these requirements
Other information:
This is a general fix to #10076 and also incorporates the changes in #14292.
While it's not a very common problem, I always found this section of the API very strange, especially since it uses an undocumented API (so if you stumble on it, it's even more difficult to find out what causes it).
Since #14292 is already a very small BC I thought a general solution to this issue would be welcome.
Note that I will include tests for the other input types if this implementation is approved.