fix(input[date]): support years with more than 4 digits#13905
fix(input[date]): support years with more than 4 digits#13905LeeAdcock wants to merge 8 commits intoangular:masterfrom LeeAdcock:input_date_fix_13735
Conversation
Change to regular expressions used to match date formats that increases the number of digits in a year. Previously exactly four digits were required, change allows four or more. Also matches complete string, resolving ambiguity around extra characters at the beginning or end.
test/ng/directive/inputSpec.js
Outdated
There was a problem hiding this comment.
You should use a different value.
|
There's no test for |
test/ng/directive/inputSpec.js
Outdated
There was a problem hiding this comment.
Actually, it must use colon separator.
… format Support for parsing additional variations of ISO dates, specifically the minute offset in timezones is now optional, as is the colon delimiter between the offset hour and minute. Enhancements to the tests were made to increase robustness and validate edge-case scenarios.
|
Thank you for the feedback, I appreciate it. |
src/ng/directive/input.js
Outdated
There was a problem hiding this comment.
Since we are not using the groups anywhere, it would be better for performance to make them non-capturing.
(As an added bonus, you'll have the chance to write a RegExp that includes ?::? - not somethng you do everyday 😁)
There was a problem hiding this comment.
BTW, I know I proposed this change, but there is currently an issue with IE/Edge (they can't parse a timezone offset when there is no colon), so additional steps might be needed. That said, I'm not sure when/if ISO_DATE_REGEXP is used after all, so let me look into it some more :)
There was a problem hiding this comment.
Likely to be a problem if the input model is populated directly with an ISO formatted text string, instead of from the browser native picker.
There was a problem hiding this comment.
I took a look at Edge in Saucelabs and found the same result when using the Date constructor for parsing both '2010-06-15T00:00:00.0000+01:30' as well as '2010-06-15T00:00:00.0000+0130'.
Is there a bug report for the Edge issue you referenced?
There was a problem hiding this comment.
I don't know if there is a bug report - it just came up a few days ago and I added a workaround to fix a similar issue in #13887.
There was a problem hiding this comment.
Likely to be a problem if the input model is populated directly with an ISO formatted text string
The ISO_DATE_REGEXP is only used in an ngModelController parser, meaning it will only be applied when parsing the $viewValue to transform it to a $modelValue. The only case where the $viewValue can be an ISO-8601 date string, is on browsers that do not natively support input[date] and the users enter it by hand (which is a rather unlikely interaction imo).
Anyway, assuming we still need ISO_DATE_REGEXP, what needs to be done in order to properly support all valid timezone offsets on all supported browsers, is "fixing up" the date string before passing it to new Date(). That would include adding a colon if missing and add 00 minutes if omitted.
Thinking about it again, this is totally unrelated to the initial intent of this PR, so it's not a good idea to mix them. I suggest, leaving only changes to add support for >4-digit years in this PR and if you fancy feel free to submit another one with the required changes to properly support all valid timezone offset formats.
Sorry, I had you make those changes and now telling they don't belong here 😞
|
@gkalpak Can you take another look at this? |
|
@Narretz, will do. Thx for the heads-up ! (I hadn't notice the updates.) |
test/ng/directive/inputSpec.js
Outdated
| expect(ISO_DATE_REGEXP.test(date)).toBe(valid); | ||
| }); | ||
|
|
||
| it('should be non-capturing', function() { |
There was a problem hiding this comment.
This test is not needed. Capturing vs Non-capturing is just an implementation detail.
There was a problem hiding this comment.
Happy to make this change, but would point out that if having the regex be non-capturing is important for performance, we should have a test that validates it. The regex was originally capturing, but that mistake wasn't noticed until this unit test was written. It's not unlikely that the next person that modifies it after us will unintentionally make it capturing again, unless there is a test enforces this style of implementation for performance gains.
There was a problem hiding this comment.
This is still an implementation detail. E.g. if we decide to add support for all timezone formats, this will change: we'll need some capturing groups. Unless it is identified as a performance issue (which it won't - it's more of a theoretical performance gain, unlikely to be noticable), it remains an implementation detail.
So, while it's nice to avoid capturing groups if you don't need them, having a test for it will only make it more difficult to add functionality later. Unit tests should assert the functionality/behavior, not the performance characteristics, imo.
|
LGTM (expect for one minor comment). |
|
Thanks for the feedback. The implementation test has been removed. |
4 participants
Change to regular expressions used for matching ISO date strings. Increases the number of digits allowed in a year from exactly four to instead be four or more. Also matches on the entire string, resolving ambiguity around extra characters at the start of the year, or after the timezone.
Closes #13735