fix(datepicker): validate that input actually parses #5711

mmalerba · 2017-07-12T23:18:53Z

BREAKING CHANGE: You must now use an actual Date object rather than a
string when setting the value of the datepicker programmatically
(through value, ngModel, or formControl).

BREAKING CHNAGE: DateAdapter subclasses must now provide an implementation for isValidDate

fixes #5417
fixes #4978

jelbourn · 2017-07-12T23:26:29Z

src/lib/core/datetime/native-date-adapter.ts

@@ -192,6 +192,10 @@ export class NativeDateAdapter extends DateAdapter<Date> {
    ].join('-');
  }

+  isDateObject(value: any) {
+    return value instanceof Date;


Add unit test for this?

jelbourn · 2017-07-12T23:27:02Z

src/lib/datepicker/datepicker-input.ts

@@ -99,12 +99,14 @@ export class MdDatepickerInput<D> implements AfterContentInit, ControlValueAcces
        this._dateFormats.parse.dateInput);
  }
  set value(value: D | null) {
-    let date = this._dateAdapter.parse(value, this._dateFormats.parse.dateInput);
+    if (value != null && !this._dateAdapter.isDateObject(value)) {
+      throw new Error('Datepicker: value not recognized as a date object by DateAdapter.');


Hot tip: with Error you don't need new (just throw Error('...'))

mmalerba · 2017-07-13T17:39:00Z

comments addressed

jelbourn

LGTM

jelbourn · 2017-07-15T01:01:36Z

Caretaker note: one google app already had an invalid value, will need to fix

mmalerba · 2017-07-25T21:03:06Z

un-lgtm'ing this so I can reconcile with what we talked about in #4978

mmalerba · 2017-07-25T21:50:24Z

@jelbourn Changed this PR a bit based on the discussion @kara and I had about #4978. I think just throwing an error like I was doing before is problematic if we want to do parse validation.

Couple of questions:

How should the calendar behave when the input has an invalid value. Currently it acts as if the input's value were null, another option would be to act as if the value had never changed from its last valid value
How should inputs behave when their from control changes to an invalid value (currently they show "INVALID DATE" which seems kind of weird...

mmalerba · 2017-07-25T21:57:04Z

src/lib/core/datetime/native-date-adapter.ts

@@ -192,6 +197,16 @@ export class NativeDateAdapter extends DateAdapter<Date> {
    ].join('-');
  }

+  isValidDate(value: any) {
+    if (value == null) {


@jelbourn It kind of feels wrong that people extending DateAdapter even have the ability to say "null is invalid" maybe an indicator that I should move all the null checks into the datepicker code itself, WDYT?

mmalerba · 2017-07-26T00:26:16Z

Ok answering my own questions:

Looked at the way type="number" inputs work when you type stuff that doesn't parse (e.g. "2e"). It looks like they are pretty much treated as being null for all purposes, so did the same.
Decided users shouldn't be able to declare null to be an invalid date, refactored it so they can't

mmalerba · 2017-07-26T00:28:50Z

src/lib/core/datetime/native-date-adapter.ts

  }

  format(date: Date, displayFormat: Object): string {
+    if (!this.isValid(date)) {
+      return 'INVALID DATE';


Not sure what the best thing to do here is (probably not this). Datepicker should never call format on an invalid date, but someone could in their code, maybe just throw an error?

Throwing an error seems reasonable to me

jelbourn · 2017-07-26T20:31:08Z

src/lib/core/datetime/date-adapter.ts

+  abstract isValid(date: D): boolean;
+
+  /**
+   * @param {obj} The object to check.


Omit braces on obj

jelbourn · 2017-07-26T21:39:44Z

src/lib/core/datetime/date-adapter.ts

+   */
+  getValidDateOrNull(obj: any): D | null {
+    return (this.isDateInstance(obj) && this.isValid(obj)) ? obj : null;
+  }


Would be be more appropriate to have this in the component code than in the adapter?

mmalerba · 2017-07-26T22:45:52Z

comments adressed, also meant to include a demo link: https://mmalerba-demo2.firebaseapp.com/datepicker

mmalerba · 2017-07-27T16:06:47Z

@kara demo behavior is still a little different from how number inputs work (compare demo link above with this plunker) http://plnkr.co/edit/GoutPJd2AXx7KPD4d0XB?p=preview any advice on how to make it a little closer to what number input does?

BREAKING CHANGE: You must now use an actual Date object rather than a string when setting the value of the datepicker programmatically (through value, ngModel, or formControl).

mmalerba · 2017-07-28T22:34:10Z

@kara @jelbourn Ok I think I've finally gotten the behavior exactly how I want it now. The material input and the native input under the "Result" section of this demo are wired up to the same ngModel. Typing an unparseable date into the material one activates the md-error but doesn't cause the value to change on the other input and doesn't emit change or input events since the error value is still treated as null. Opening the calendar when the input value is unparesable still allows choosing a date as normal and just acts as if the input were null.

kara

LGTM, just nits

kara · 2017-07-31T17:01:36Z

src/lib/core/datetime/native-date-adapter.spec.ts

+  it('should parse invalid value as invalid', () => {
+    let d = adapter.parse('hello');
+    expect(d).not.toBeNull();
+    expect(adapter.isDateInstance(d)).toBe(true);


Nit: add a failure message here? Seems odd without context that 'hello' would be a Date instance. e.g. Expected string to be fed through Date.parse()

kara · 2017-07-31T17:10:20Z

src/lib/datepicker/datepicker.spec.ts

+      it('should throw when given wrong data type', () => {
+        testComponent.date = '1/1/2017' as any;
+
+        expect(() => fixture.detectChanges()).toThrow();


Nit: Add a regex to make this clear which error should surface? e.g. toThrowError(new RegExp(...)

kara · 2017-07-31T17:19:58Z

src/lib/core/datetime/native-date-adapter.ts

  }

  format(date: Date, displayFormat: Object): string {
+    if (!this.isValid(date)) {
+      return 'INVALID DATE';


Throwing an error seems reasonable to me

mmalerba · 2017-07-31T17:44:33Z

Comments addressed

kara · 2017-07-31T17:46:40Z

LGTM

uvconnects · 2017-08-31T18:39:35Z

I also get this when I was not getting this before date format is 2017-08-02T00:00:00

jbojcic1 · 2017-12-12T23:16:41Z

@mmalerba in your demo, validation is passing if I type 34 and I get 1/1/2034

mmalerba · 2017-12-13T21:36:54Z

@jbojcic1 that's because Date.parse('34') parses to 1/1/2034. The NativeDateAdapter simply delegates parsing to Date.parse. If you want to change the parsing behavior you can create a custom DateAdapter

angular-automatic-lock-bot · 2019-09-08T16:02:43Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mmalerba added the pr: needs review label Jul 12, 2017

mmalerba assigned jelbourn Jul 12, 2017

mmalerba requested a review from jelbourn July 12, 2017 23:18

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 12, 2017

jelbourn reviewed Jul 13, 2017

View reviewed changes

mmalerba force-pushed the dp-err branch from bbac839 to 6238f2c Compare July 13, 2017 17:38

jelbourn approved these changes Jul 13, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed pr: needs review labels Jul 13, 2017

mmalerba mentioned this pull request Jul 17, 2017

md-datepicker throw exception #5417

Closed

mmalerba force-pushed the dp-err branch from 6238f2c to 1ebf719 Compare July 21, 2017 23:18

mmalerba removed pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 25, 2017

mmalerba force-pushed the dp-err branch from 1ebf719 to cb2ddec Compare July 25, 2017 21:36

mmalerba changed the title ~~fix(datepicker): throw when value is set to invalid type~~ fix(datepicker): validate that input actually parses Jul 25, 2017

mmalerba requested a review from kara July 25, 2017 21:40

mmalerba assigned kara Jul 25, 2017

mmalerba added the pr: needs review label Jul 25, 2017

mmalerba force-pushed the dp-err branch from cb2ddec to dad819c Compare July 25, 2017 21:45

mmalerba commented Jul 25, 2017

View reviewed changes

mmalerba force-pushed the dp-err branch 2 times, most recently from 9fd24e0 to 34bc1be Compare July 26, 2017 00:22

mmalerba commented Jul 26, 2017

View reviewed changes

jelbourn reviewed Jul 26, 2017

View reviewed changes

mmalerba added 8 commits July 28, 2017 14:49

fix(datepicker): throw when value is set to invalid type

3a06481

BREAKING CHANGE: You must now use an actual Date object rather than a string when setting the value of the datepicker programmatically (through value, ngModel, or formControl).

addressed comments

9013aef

validate that datepicker input parses

042ac8a

tweak handling of invalid dates.

c7dd93f

fix tests

0477a90

address feedback

4c8ace2

make sure datepicker selected date is valid

da045a6

rework validation again

69eef51

mmalerba force-pushed the dp-err branch from 524c28e to 69eef51 Compare July 28, 2017 22:26

kara approved these changes Jul 31, 2017

View reviewed changes

addressed comments

328153a

kara added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jul 31, 2017

kara removed their assignment Jul 31, 2017

fix lint

245d239

tinayuangao merged commit 8bb54ca into angular:master Aug 1, 2017

mmalerba deleted the dp-err branch April 3, 2018 15:18

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(datepicker): validate that input actually parses #5711

fix(datepicker): validate that input actually parses #5711

mmalerba commented Jul 12, 2017 •

edited

jelbourn Jul 12, 2017

jelbourn Jul 12, 2017

mmalerba commented Jul 13, 2017

jelbourn left a comment

jelbourn commented Jul 15, 2017 •

edited

mmalerba commented Jul 25, 2017

mmalerba commented Jul 25, 2017

mmalerba Jul 25, 2017

mmalerba commented Jul 26, 2017

mmalerba Jul 26, 2017

kara Jul 31, 2017

jelbourn Jul 26, 2017

jelbourn Jul 26, 2017

mmalerba commented Jul 26, 2017

mmalerba commented Jul 27, 2017

mmalerba commented Jul 28, 2017

kara left a comment

kara Jul 31, 2017

kara Jul 31, 2017

kara Jul 31, 2017

mmalerba commented Jul 31, 2017

kara commented Jul 31, 2017

uvconnects commented Aug 31, 2017

jbojcic1 commented Dec 12, 2017

mmalerba commented Dec 13, 2017

angular-automatic-lock-bot bot commented Sep 8, 2019

fix(datepicker): validate that input actually parses #5711

fix(datepicker): validate that input actually parses #5711

Conversation

mmalerba commented Jul 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Jul 13, 2017

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn commented Jul 15, 2017 • edited

mmalerba commented Jul 25, 2017

mmalerba commented Jul 25, 2017

Choose a reason for hiding this comment

mmalerba commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Jul 26, 2017

mmalerba commented Jul 27, 2017

mmalerba commented Jul 28, 2017

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Jul 31, 2017

kara commented Jul 31, 2017

uvconnects commented Aug 31, 2017

jbojcic1 commented Dec 12, 2017

mmalerba commented Dec 13, 2017

angular-automatic-lock-bot bot commented Sep 8, 2019

mmalerba commented Jul 12, 2017 •

edited

jelbourn commented Jul 15, 2017 •

edited