fix(datepicker): allow `DateAdapter` authors to have more control ove… #7346

mmalerba · 2017-09-26T23:10:55Z

…r what can/can't be coerced to a date

fixes #7286

BREAKING CHANGES:

fromIso8601 method on DateAdapter removed in favor of deserialize
DateAdapter implementations must now implement the invalid method

mmalerba · 2017-10-02T00:36:30Z

rebased

jelbourn · 2017-10-08T15:49:26Z

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

+   *     null date (e.g. the empty string).
+   * @throws If the given value cannot be coerced to a valid date or null.
+   */
+  coerceToDate(value: any): D | null {


Doesn't this need to align with toIso8601? I think the heart of what this is trying to do is the same as parse / format except that instead of being string that the user see / inputs, it's the formats that the backend sends / accepts. Looking at it that way, I think this would make sense:

parse / format -> for user-facing strings
serialize / deserialize -> for backend-facing strings (or JSON or whatever)

For user-facing strings: I agree we need both.

For backend-facing strings: I don't think the DateAdapter is particularly concerned with these for the most part. We don't really know if the dates coming into / out of the datepicker are going to the backend or some other part of the JS app, its up to the user to decide and handle that. However for convenience we might want to allow certain values (such as ISO 8601 strings or UNIX timestamps) to be automatically coerced into the proper type - similar to how we coerce "true" and "false" for boolean properties. This is what the new coerceToDate method is about, allowing the user to decide what can be coerced and what can't.

There's also a third class: DOM-facing strings. For example to set the min attribute on the input we need the date in a format that the DOM can understand, which is ISO 8601. This is why we always need to have a way to go from whatever the date type is to an ISO 8601 string.

How do you feel, then, about calling this method deserialize?

Also on toIso8601, we should document somewhere why/how this is used.

👍

I'll update the description of all the to/from date methods to make sure its clear exactly what each one is used for

mmalerba · 2017-10-12T17:18:01Z

comments addressed

jelbourn · 2017-10-12T18:10:20Z

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

@@ -118,7 +118,7 @@ export abstract class DateAdapter<D> {
  abstract today(): D;

  /**
-   * Parses a date from a value.
+   * Parses a date from a user-inputted value.


user-provided?

jelbourn · 2017-10-12T18:11:41Z

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

+   * the given value is already a valid date object or null. The `<mat-datepicker>` will call this
+   * method on all of it's `@Input()` properties that accept dates. It is therefore possible to
+   * support passing your wire format directly to these properties by overriding this method to
+   * also deserialize your wire format.


Say something like "This will generally be used for data coming directory from some backend or database." ?

Isn't this just saying the same thing as the "wire format" part above?

Yeah, but it's a bit more explicit (less jargony?) than "wire format"

Ok, re-worded it to say backend

jelbourn · 2017-10-12T18:27:24Z

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

@@ -192,6 +187,26 @@ export abstract class DateAdapter<D> {
  abstract isValid(date: D): boolean;

  /**
+   * Attempts to deserialize a value to a valid date object. This is different from parsing in that
+   * deserialize should only accept non-ambiguous, locale-independent values (e.g. a ISO 8601


"locale-independent values" -> "locale-independent formats"?

Does the phrase "locale-independent" refer specifically to the format? E.g., ISO 8601 not a localized format, but it might not contain UTC

mmalerba · 2017-10-12T18:55:21Z

done

jelbourn · 2017-10-12T19:24:00Z

Caretaker note: will need to update APIs for folks that have their own adapter (at least one).

mmalerba · 2017-10-24T21:32:24Z

Instantly throwing when deserialization fails may be a little harsh. I think I'll rework this to return an invalid date or something

…r what can/can't be coerced to a date BREAKING CHANGES: - `fromIso8601` method on `DateAdapter` removed in favor of `coerceToDate`

mmalerba · 2017-10-27T18:35:06Z

@jelbourn made some changes, PTAL

jelbourn

LGTM

Elkrival · 2017-11-01T18:17:14Z

@mmalerba Thank you for the adapter example code, it asserts the methods to implementing MomentJS to the Datepicker Module.

I am adding the DatePickerModule to my current project, when implementing the new changes from the DateAdapter example code, I keep running into an error in line 242 where return super.deserialize(value) is not found in the Moment export instance. As a workaround I reverted back to the code before the deserialize() function was added.

A thousand thank you's for the example, we love the material2 library, and my designer friend Chandan loves the Material Design standard.

angular-automatic-lock-bot · 2019-09-09T08:02:58Z

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 P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful pr: needs review labels Sep 26, 2017

mmalerba assigned jelbourn Sep 26, 2017

mmalerba requested a review from jelbourn September 26, 2017 23:10

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

mmalerba force-pushed the dp-coerce branch from 083fab0 to 1535594 Compare September 27, 2017 19:55

BenRacicot mentioned this pull request Sep 28, 2017

Can't bind to 'mdDatepickerToggle' since it isn't a known property of 'button'. #6186

Closed

mmalerba force-pushed the dp-coerce branch from 1535594 to 1f1a540 Compare October 2, 2017 00:36

mmalerba force-pushed the dp-coerce branch from 1f1a540 to 432512c Compare October 6, 2017 03:20

jelbourn reviewed Oct 8, 2017

View reviewed changes

mmalerba force-pushed the dp-coerce branch 2 times, most recently from 9051f65 to 227e9c9 Compare October 11, 2017 22:58

jelbourn reviewed Oct 12, 2017

View reviewed changes

mmalerba force-pushed the dp-coerce branch from 227e9c9 to 0823d4a Compare October 12, 2017 18:54

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

mmalerba force-pushed the dp-coerce branch 2 times, most recently from 84fc26d to 8166e61 Compare October 13, 2017 18:01

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Oct 24, 2017

mmalerba added 4 commits October 27, 2017 11:02

fix(datepicker): allow DateAdapter authors to have more control ove…

74a764c

…r what can/can't be coerced to a date BREAKING CHANGES: - `fromIso8601` method on `DateAdapter` removed in favor of `coerceToDate`

address comments

468a0ab

more comments addressed

a31aec4

return invalid date instead of throwing

a4585d6

mmalerba force-pushed the dp-coerce branch from 8166e61 to a4585d6 Compare October 27, 2017 18:34

mmalerba removed the pr: lgtm label Oct 27, 2017

mmalerba mentioned this pull request Oct 27, 2017

fix(datepicker): corrected logic when determining if date is valid (#8045) #8062

Closed

jelbourn approved these changes Oct 27, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 27, 2017

mmalerba merged commit 9fa075e into angular:master Oct 30, 2017

mmalerba deleted the dp-coerce branch July 31, 2018 21:19

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(datepicker): allow `DateAdapter` authors to have more control ove… #7346

fix(datepicker): allow `DateAdapter` authors to have more control ove… #7346

mmalerba commented Sep 26, 2017 •

edited

Loading

mmalerba commented Oct 2, 2017

jelbourn Oct 8, 2017

mmalerba Oct 9, 2017

jelbourn Oct 11, 2017

mmalerba Oct 11, 2017

mmalerba commented Oct 12, 2017

jelbourn Oct 12, 2017

jelbourn Oct 12, 2017

mmalerba Oct 12, 2017

jelbourn Oct 12, 2017

mmalerba Oct 12, 2017

jelbourn Oct 12, 2017

mmalerba commented Oct 12, 2017

jelbourn commented Oct 12, 2017

mmalerba commented Oct 24, 2017

mmalerba commented Oct 27, 2017

jelbourn left a comment

Elkrival commented Nov 1, 2017 •

edited

Loading

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

fix(datepicker): allow DateAdapter authors to have more control ove… #7346

fix(datepicker): allow DateAdapter authors to have more control ove… #7346

Conversation

mmalerba commented Sep 26, 2017 • edited Loading

mmalerba commented Oct 2, 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 Oct 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Oct 12, 2017

jelbourn commented Oct 12, 2017

mmalerba commented Oct 24, 2017

mmalerba commented Oct 27, 2017

jelbourn left a comment

Choose a reason for hiding this comment

Elkrival commented Nov 1, 2017 • edited Loading

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

fix(datepicker): allow `DateAdapter` authors to have more control ove… #7346

fix(datepicker): allow `DateAdapter` authors to have more control ove… #7346

mmalerba commented Sep 26, 2017 •

edited

Loading

Elkrival commented Nov 1, 2017 •

edited

Loading