feat(moment-dateadapter): add option to create utc dates #11336

Silthus · 2018-05-15T10:42:54Z

Moment DateAdapter should create dates in utc format to avoid time zone
collisions when passing date to servers.
To avoid a breaking change a new injection token MAT_MOMENT_DATE_ADAPTER_OPTIONS has been created with the option useUtc: boolean. The option defaults to false.

See #7167 (comment) for more details on the issue.

Closes #7167

Moment DateAdapter should create dates in utc format to avoid time zone collisions when passing date to servers Closes angular#7167

googlebot · 2018-05-15T10:42:56Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Silthus · 2018-05-15T10:55:39Z

I added the missing email addresses to my google and github account. CLA should be fine now.

googlebot · 2018-05-15T10:55:41Z

CLAs look good, thanks!

sir-boformer · 2018-05-17T20:53:37Z

Very welcome addition!

This will break existing apps which depend on the local date output.

mmalerba · 2018-05-21T19:15:05Z

I initially thought this was doing something similar to what we do in NativeDateAdapter where we re-create the date as a UTC date for display purposes only, but this is actually changing the format passed to and from the user. I don't think we want to make this change since it will break a lot of apps. If you need to pass the object to a server just call .utc() on the object passed back by the datepicker

eitland · 2018-05-22T17:55:46Z

@mmalerba, thanks for looking into this, I think however the issue is more complicated.

My, and I think a number of others problem is that it parses wrong.

you say:

If you need to pass the object to a server just call .utc() on the object passed back by the datepicker

here is what moment js docs says:

moment().utc();

Sets a flag on the original moment to use UTC to display a moment instead of the original moment's time.

So when I click on the date 2018-05-19, the moment object arrives as "2018-01-18T23:00:00Z", i.e. it points to the wrong timestamp.

I don't think we want to make this change since it will break a lot of apps.

Very probably yes, but on the other hand I've also seen at least one other workaround similar to the one @Silthus proposes, and I'm going to have to implement something similar as well.

Could we have a UTCMomentDateAdapter in addition to the current MomentDateAdapter? Or maybe we could add a flag somehow?

mmalerba · 2018-05-22T18:23:33Z

I see, we could do it as an option probably. We could create injectable a token like MAT_MOMENT_DATE_ADAPTER_OPTIONS which would be an object {useUtc: boolean}, default would be false, but setting it to true will enable the behavior you guys are looking for. I'll reopen this PR so @Silthus can make this change

Silthus · 2018-06-04T16:54:05Z

I still have this on my radar and will try to work on it this week 👍🏻

Added MAT_MOMENT_DATE_ADAPTER_OPTIONS with the useUtc: boolean option to parse dates as utc The new option defaults to false and will not cause any breaking changes. Closes angular#7167

Describe how to set the MomentDateAdapter to parse dates as utc. Describe the default behaviour of the MomentDateAdapter. Add an example on how to configure MAT_MOMENT_DATA_ADAPTER_OPTIONS Closes angular#7167

Silthus · 2018-06-05T10:20:21Z

@mmalerba is it ok now like this?

mmalerba · 2018-06-05T14:41:40Z

src/material-moment-adapter/adapter/index.ts

+      deps: [MAT_DATE_LOCALE, MAT_MOMENT_DATE_ADAPTER_OPTIONS]
+    },
+    {
+      provide: MAT_MOMENT_DATE_ADAPTER_OPTIONS,


@jelbourn do you think we should use a root or module provider for this?

Probably root, but my tests failed with just the 'root' keyword on the injection token. Not sure how to do that with the new angular 6 system.

Generally anything that doesn't depend on Overlay should be providedIn: 'root'

You should be able to use MAT_DATE_LOCALE as an example: https://github.com/angular/material2/blob/master/src/lib/core/datetime/date-adapter.ts#L13

And tests for it here: https://github.com/angular/material2/blob/master/src/lib/core/datetime/native-date-adapter.spec.ts

It looks like you set it up correctly... what error were you getting in the tests?

Only provide MAT_MOMENT_DATE_ADAPTER_OPTIONS in root

Silthus · 2018-06-06T05:51:42Z

src/material-moment-adapter/adapter/index.ts

@@ -29,10 +29,6 @@ export * from './moment-date-formats';
      provide: DateAdapter,
      useClass: MomentDateAdapter,
      deps: [MAT_DATE_LOCALE, MAT_MOMENT_DATE_ADAPTER_OPTIONS]


I think I found why the tests did not work. Now with the MAT_MOMENT_DATE_ADAPTER_OPTIONS in the deps all tests pass.

Only provide MAT_MOMENT_DATE_ADAPTER_OPTIONS in root

…us/material2 into fix-moment-dateadapter-utc

Silthus · 2018-06-06T08:52:21Z

@mmalerba should all be good now. The travis check failed because of a timeout to the edge browser.

mmalerba

Looks good, we just need to make the new constructor param optional so its not a breaking change

mmalerba · 2018-06-06T18:26:00Z

src/material-moment-adapter/adapter/moment-date-adapter.ts

@@ -48,7 +73,8 @@ export class MomentDateAdapter extends DateAdapter<Moment> {
    narrowDaysOfWeek: string[]
  };

-  constructor(@Optional() @Inject(MAT_DATE_LOCALE) dateLocale: string) {
+  constructor(@Optional() @Inject(MAT_DATE_LOCALE) dateLocale: string,
+    @Inject(MAT_MOMENT_DATE_ADAPTER_OPTIONS) private options: MatMomentDateAdapterOptions) {


In order to make this not a breaking change for anyone who might be extending the MomentDateAdapter lets make this:

/** @deletion-target 8.0.0 make this param required */ @Optional() @Inject(MAT_MOMENT_DATE_ADAPTER_OPTIONS) private options?: MatMomentDateAdapterOptions

mmalerba · 2018-06-06T18:26:34Z

src/material-moment-adapter/adapter/moment-date-adapter.ts

@@ -130,7 +156,12 @@ export class MomentDateAdapter extends DateAdapter<Moment> {
      throw Error(`Invalid date "${date}". Date has to be greater than 0.`);
    }

-    let result = moment({year, month, date}).locale(this.locale);
+    let result;
+    if (this.options.useUtc) {


this.options && this.options.useUtc after making the change requested above

Silthus · 2018-06-08T15:11:15Z

@mmalerba done

eitland · 2018-06-13T21:41:24Z

@mmalerba care to look into it? This should solve an a real problem for many of us.

kuncevic · 2018-06-19T00:30:24Z

Is it coming in the next release?

mmalerba · 2018-06-19T04:55:00Z

@kuncevic It's ready to submit, but we need to check it against Google's codebase to make sure nothing breaks first

sheikalthaf · 2018-06-24T05:22:34Z

@mmalerba hi,
May i know when this pull request will be merged. This is one of my main feature which i lagging behind in my project

ghost · 2018-06-28T15:23:13Z

Will this also work with manually typed in dates? If not, what should be used for the parse.dateInput of MAT_DATE_FORMATS?

ghost · 2018-06-28T20:51:53Z

I'm pretty sure that the parse function will need replacing with something like:

  parse(value: any, parseFormat: string | string[]): Moment | null {
    if (value && typeof value === 'string') {
      if (this.options && this.options.useUtc) {
        return moment.utc(value, parseFormat, this.locale, true);
      } else {
        return moment(value, parseFormat, this.locale, true);
      }
    }
    if (this.options && this.options.useUtc) {
      return value ? moment.utc(value).locale(this.locale) : null;
    } else {
      return value ? moment(value).locale(this.locale) : null;
    }
  }

There could be other places where moment will need replacing with moment.utc and maybe a neater way of replacing them all. Sorry for not submitting a pull request right now.

angular-automatic-lock-bot · 2019-09-09T04:02:28Z

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.}

fix(dateadapter): moment dates not in utc format

1db44c3

Moment DateAdapter should create dates in utc format to avoid time zone collisions when passing date to servers Closes angular#7167

Silthus requested a review from mmalerba as a code owner May 15, 2018 10:42

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label May 15, 2018

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels May 15, 2018

style(lint): break line at 100 chars

e2fe907

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels May 15, 2018

mmalerba approved these changes May 15, 2018

View reviewed changes

mmalerba removed pr: lgtm action: merge The PR is ready for merge by the caretaker labels May 21, 2018

mmalerba closed this May 21, 2018

mmalerba reopened this May 22, 2018

Silthus changed the title ~~fix(dateadapter): moment dates not in utc format~~ feat(moment-dateadapter): add option to create utc dates Jun 5, 2018

Silthus added 3 commits June 5, 2018 11:18

feat(moment-dateadapter): add option to create utc dates

1577506

Added MAT_MOMENT_DATE_ADAPTER_OPTIONS with the useUtc: boolean option to parse dates as utc The new option defaults to false and will not cause any breaking changes. Closes angular#7167

docs(moment-dateadapter): add docs for moment adapter options

d0abfc9

Describe how to set the MomentDateAdapter to parse dates as utc. Describe the default behaviour of the MomentDateAdapter. Add an example on how to configure MAT_MOMENT_DATA_ADAPTER_OPTIONS Closes angular#7167

style(moment-dateadapter): fix lint styling

83ba2a0

mmalerba reviewed Jun 5, 2018

View reviewed changes

refactor(moment-dateadapter): remove module provider

cda28a1

Only provide MAT_MOMENT_DATE_ADAPTER_OPTIONS in root

Silthus commented Jun 6, 2018

View reviewed changes

Silthus added 2 commits June 6, 2018 07:58

refactor(moment-dateadapter): remove module provider

abfb4e0

Only provide MAT_MOMENT_DATE_ADAPTER_OPTIONS in root

Merge branch 'fix-moment-dateadapter-utc' of https://github.com/Silth…

fa8ad76

…us/material2 into fix-moment-dateadapter-utc

mmalerba reviewed Jun 6, 2018

View reviewed changes

refactor(material-dateadapter): make moment dateadapter options optional

c7c14fb

mmalerba approved these changes Jun 15, 2018

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 15, 2018

josephperrott added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Jun 27, 2018

josephperrott merged commit 9a85b9b into angular:master Jun 27, 2018

josephperrott pushed a commit to josephperrott/components that referenced this pull request Jun 27, 2018

feat(moment-dateadapter): add option to create utc dates (angular#11336)

3acd2a4

victoriaaa234 pushed a commit that referenced this pull request Jul 25, 2018

feat(moment-dateadapter): add option to create utc dates (#11336)

28f1ed8

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

feat(moment-dateadapter): add option to create utc dates #11336

feat(moment-dateadapter): add option to create utc dates #11336

Silthus commented May 15, 2018 •

edited

Loading

googlebot commented May 15, 2018

Silthus commented May 15, 2018

googlebot commented May 15, 2018

sir-boformer commented May 17, 2018

mmalerba commented May 21, 2018

eitland commented May 22, 2018

mmalerba commented May 22, 2018

Silthus commented Jun 4, 2018

Silthus commented Jun 5, 2018

mmalerba Jun 5, 2018

Silthus Jun 5, 2018

jelbourn Jun 5, 2018

mmalerba Jun 5, 2018

Silthus Jun 6, 2018

Silthus commented Jun 6, 2018

mmalerba left a comment

mmalerba Jun 6, 2018

mmalerba Jun 6, 2018

Silthus commented Jun 8, 2018

eitland commented Jun 13, 2018

kuncevic commented Jun 19, 2018

mmalerba commented Jun 19, 2018

sheikalthaf commented Jun 24, 2018

ghost commented Jun 28, 2018

ghost commented Jun 28, 2018

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

feat(moment-dateadapter): add option to create utc dates #11336

feat(moment-dateadapter): add option to create utc dates #11336

Conversation

Silthus commented May 15, 2018 • edited Loading

googlebot commented May 15, 2018

Silthus commented May 15, 2018

googlebot commented May 15, 2018

sir-boformer commented May 17, 2018

mmalerba commented May 21, 2018

eitland commented May 22, 2018

mmalerba commented May 22, 2018

Silthus commented Jun 4, 2018

Silthus commented Jun 5, 2018

mmalerba Jun 5, 2018

Choose a reason for hiding this comment

Silthus Jun 5, 2018

Choose a reason for hiding this comment

jelbourn Jun 5, 2018

Choose a reason for hiding this comment

mmalerba Jun 5, 2018

Choose a reason for hiding this comment

Silthus Jun 6, 2018

Choose a reason for hiding this comment

Silthus commented Jun 6, 2018

mmalerba left a comment

Choose a reason for hiding this comment

mmalerba Jun 6, 2018

Choose a reason for hiding this comment

mmalerba Jun 6, 2018

Choose a reason for hiding this comment

Silthus commented Jun 8, 2018

eitland commented Jun 13, 2018

kuncevic commented Jun 19, 2018

mmalerba commented Jun 19, 2018

sheikalthaf commented Jun 24, 2018

ghost commented Jun 28, 2018

ghost commented Jun 28, 2018

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

Silthus commented May 15, 2018 •

edited

Loading