fix(dateparser): add type and validity check #3759

wesleycho · 2015-06-04T19:34:44Z

Add type and validity check to ensure proper date object is created
Make code style uniform in scripts

Plunker with fix in action here

This addresses #3713

wesleycho · 2015-06-04T22:08:43Z

I believe this also closes #3701, and fixes #3609, #3736, #3071.

rvanbaalen · 2015-06-05T02:55:43Z

Leaving a comment to keep track on this one. Will review this saturday.

chrisirhc · 2015-06-07T18:07:31Z

@wesleycho , there's a lot of whitespace changes in this PR. Any chance of editing that away? Just want to avoid adding noise to git blame.

chrisirhc · 2015-06-07T18:11:37Z

Code change looks good to me.
I was only wondering if this should log a warning when the base date argument is provided but invalid (isNaN check returns true).

wesleycho · 2015-06-07T18:27:25Z

Sure, I can do that sometime later today or tomorrow.

I can add a $log.debug or $log.warn statement about a string where isNaN returns true and falling back to date constructor.

wesleycho · 2015-06-08T01:49:48Z

I reverted all code style changes

chrisirhc · 2015-06-08T04:43:29Z

src/dateparser/dateparser.js

@@ -136,6 +136,9 @@ angular.module('ui.bootstrap.dateparser', [])
          milliseconds: baseDate.getMilliseconds()
        };
      } else {
+        if (baseDate) {
+          $log.warn('dateparser:', 'baseDate is a string with no date representation');


How about baseDate is not a valid date ?

Much better than what I had - changing shortly

- Add type and validity check to ensure proper date object is created - Make code style uniform in scripts chore(dateparser): revert code style changes chore(dateparser): add `$log.warn`

chrisirhc · 2015-06-08T17:44:49Z

Hm, I was just testing this, I think this introduces another bug, now there's time added to the string, possibly due to a toISOString conversion that shouldn't be done:

See http://plnkr.co/edit/XCvfhGTYUUraB6mTSVSK?p=preview (there are steps to reproduce on the Plunker)

I'm thinking the change in #3294 is needed to actually fix this at the root.

wesleycho · 2015-06-09T17:05:47Z

I tried some experimenting, and it looks like stripping out the || new Date(...) portions from the $viewChangeListener and $parser does not fix this issue at all. In fact, this looks to be a preexisting issue in the 0.13.0 release itself, due to the fact that the view value gets converted to a date as the model value.

wesleycho · 2015-06-10T14:01:52Z

#3643 may also be related.

cbosco · 2015-06-24T19:10:16Z

+1

IRus · 2015-07-16T11:13:24Z

Hi, don't understand this check:

if (angular.isDate(baseDate) && !isNaN(baseDate.getTime())) {

if baseDate is date getTime always will return an number, right?

wesleycho · 2015-07-16T13:53:45Z

That is not true - try doing new Date('foo').getTime()

IRus · 2015-07-16T13:55:27Z

Yep, you right, Thank you for example.

wesleycho · 2015-07-20T12:55:43Z

@chrisirhc what are your thoughts on this? I do not think that the model aspect is fixable, since the model value will always become a date object or null currently (even if the new Date(...) fallback is stripped).

This can potentially be alleviated if we are able to switch to allowing users to bring in a custom parser & formatter for converting between a user desired model and the view value, which would also open up moment.js support for users, but that is a large & separate task.

I think this PR should make it in in its current incarnation though - it fixes a badly breaking bug in the datepicker.

IRus · 2015-07-20T12:58:09Z

@wesleycho please remove || new Date(...) "fallback" that actually introducing new bugs.

wesleycho · 2015-07-20T12:59:09Z

That is a breaking change that has explicit tests written against it - it is outside the scope of this PR.

anilnatha · 2015-07-23T15:32:45Z

Is this in any way related to Issue #3617?

Reason I ask is because one of the behaviors of the bug illustrated in 3617 is that when you clear a datepicker field, it sets a form's $invalid property to false, even though the field is blank and the field is marked as not required. I created a plunker illustrating the issue and is posted in that thread.

llafuente · 2015-07-27T11:31:31Z

Partially related.
I apply this patch, and the exception is gone, but form is still $invalid.

wesleycho · 2015-07-28T03:47:16Z

@llafuente I am not seeing the form being invalid incorrectly - here is an updated Plunker.

llafuente · 2015-07-28T08:16:35Z

This patch alone do not solve: #3617

I apply this patch to 0.13, and didn't work, form was $invalid.
I can confirm master + this patch works.

wesleycho · 2015-07-28T12:52:03Z

master is the only version that matters. There are a lot of changes since 0.13.0.

wesleycho · 2015-07-31T16:05:42Z

Merging this in shortly, as this check in itself is completely safe & necessary.

wesleycho added component: datepicker needs: review labels Jun 4, 2015

wesleycho added this to the 0.13.1 (Performance) milestone Jun 4, 2015

wesleycho mentioned this pull request Jun 4, 2015

dateParser needs to handle invalid base dates #3708

Closed

wesleycho mentioned this pull request Jun 5, 2015

Validation issue e.g. 2015-02-30 #3342

Closed

chrisirhc added AngularJS 1.2 needs: work and removed needs: review AngularJS 1.2 labels Jun 7, 2015

wesleycho force-pushed the fix/dateparser-graceful-conditionals branch 3 times, most recently from afe69ca to 1fd0b80 Compare June 8, 2015 01:49

wesleycho added needs: review and removed needs: work labels Jun 8, 2015

wesleycho force-pushed the fix/dateparser-graceful-conditionals branch 2 times, most recently from 09919d0 to b59efc1 Compare June 8, 2015 01:55

chrisirhc reviewed Jun 8, 2015
View reviewed changes

fix(dateparser): add type and validity check

3dbfb80

- Add type and validity check to ensure proper date object is created - Make code style uniform in scripts chore(dateparser): revert code style changes chore(dateparser): add `$log.warn`

wesleycho force-pushed the fix/dateparser-graceful-conditionals branch from b59efc1 to 3dbfb80 Compare June 8, 2015 12:57

wesleycho mentioned this pull request Jun 14, 2015

fix(dateParser): handle cases where baseDate is an Invalid Date #3817

Closed

This was referenced Jul 2, 2015

Using alternative formatting breaks validation #3875

Closed

Fix for issue #3875 #3876

Closed

wesleycho mentioned this pull request Jul 14, 2015

Make sure baseDate is actually a valid date. #3933

Closed

This was referenced Jul 17, 2015

datetimepicker - wrong validatio #3937

Closed

Fix for: https://github.com/angular-ui/bootstrap/issues/3701 #3971

Closed

RobJacobs mentioned this pull request Jul 22, 2015

Datepicker #3976

Closed

wesleycho modified the milestones: 0.13.1 (npm), 0.13.2 (Performance) Jul 23, 2015

This was referenced Jul 28, 2015

V0.13 - Cannot type only day with popup datepicker #3869

Closed

Fixed bug "datetimepicker - wrong validatio #3937" #4035

Closed

wesleycho removed the needs: review label Jul 31, 2015

wesleycho closed this in 4f1e03f Jul 31, 2015

wesleycho deleted the fix/dateparser-graceful-conditionals branch July 31, 2015 16:25

wesleycho mentioned this pull request Aug 1, 2015

datepicker 0.13.0 does not allow model updates after a parse error for certain dates when ng-model-options updateOn: blur is used #3643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dateparser): add type and validity check #3759

fix(dateparser): add type and validity check #3759

wesleycho commented Jun 4, 2015

wesleycho commented Jun 4, 2015

rvanbaalen commented Jun 5, 2015

chrisirhc commented Jun 7, 2015

chrisirhc commented Jun 7, 2015

wesleycho commented Jun 7, 2015

wesleycho commented Jun 8, 2015

chrisirhc Jun 8, 2015

wesleycho Jun 8, 2015

chrisirhc commented Jun 8, 2015

wesleycho commented Jun 9, 2015

wesleycho commented Jun 10, 2015

cbosco commented Jun 24, 2015

IRus commented Jul 16, 2015

wesleycho commented Jul 16, 2015

IRus commented Jul 16, 2015

wesleycho commented Jul 20, 2015

IRus commented Jul 20, 2015

wesleycho commented Jul 20, 2015

anilnatha commented Jul 23, 2015

llafuente commented Jul 27, 2015

wesleycho commented Jul 28, 2015

llafuente commented Jul 28, 2015

wesleycho commented Jul 28, 2015

wesleycho commented Jul 31, 2015

fix(dateparser): add type and validity check #3759

fix(dateparser): add type and validity check #3759

Conversation

wesleycho commented Jun 4, 2015

wesleycho commented Jun 4, 2015

rvanbaalen commented Jun 5, 2015

chrisirhc commented Jun 7, 2015

chrisirhc commented Jun 7, 2015

wesleycho commented Jun 7, 2015

wesleycho commented Jun 8, 2015

chrisirhc Jun 8, 2015

Choose a reason for hiding this comment

wesleycho Jun 8, 2015

Choose a reason for hiding this comment

chrisirhc commented Jun 8, 2015

wesleycho commented Jun 9, 2015

wesleycho commented Jun 10, 2015

cbosco commented Jun 24, 2015

IRus commented Jul 16, 2015

wesleycho commented Jul 16, 2015

IRus commented Jul 16, 2015

wesleycho commented Jul 20, 2015

IRus commented Jul 20, 2015

wesleycho commented Jul 20, 2015

anilnatha commented Jul 23, 2015

llafuente commented Jul 27, 2015

wesleycho commented Jul 28, 2015

llafuente commented Jul 28, 2015

wesleycho commented Jul 28, 2015

wesleycho commented Jul 31, 2015