Improve timezone handling in service's moment() method

[algodave]

I'm using moment service in my app, and setting timezone globally as explained in Readme.

My app is invoking the moment method provided by the above service; however, when I pass it a formatted date with no time info (e.g. 2018-09-24), it's not working as expected. That because such method creates a moment object first (which inherits timezone from browser's locale), and then sets the timezone on it, when present. I think it should rather create the moment object using moment.tz in the beginning, when timeZone is present.

Here's how moment.js behaves when I create a moment object from a date string, and then later assign a timezone (different from my locale's one) to it:

Here's how moment.js behaves when I create a moment object from a date string, but setting the timezone since creation:

The latter is the behavior I would expect when invoking the moment() method on the addon-provided moment service, when passing a date string like the above.

What do you guys think?

[ntgussoni]

Hello, for what I saw yesterday the timeZone that the service handles is being injected on all helpers and when you use the moment method provided in the service. That works allright.

What i've found is that if you try to use moment without calling the moment method in the service then the timezone is not set. You have to call moment.tz.setDefault('America/Denver'); in order to get it right in all places.

Is this the desired behaviour? Why not just set the default timezone for moment inside the changeTimeZone method instead of keeping it in a property and applying to all moment calls?

Cheers

[algodave]

@ntgussoni Maybe my explanation of the problem wasn't clear enough... It's totally fine with me to keep the timezone in a property.

Invoking moment.tz.setDefault() won't solve the issue because after invoking it, any moment object created from that point on will use that timezone; I don't like that, I prefer the freedom to set the timezone on each individual moment object at the time I create it

What I would suggest is to change the way the moment object is created in the moment() method of moment service. At the time of writing, it looks like this (on master branch):

  moment() {
    let momentObj = moment(...arguments);
    let { locale, timeZone } = getProperties(this, 'locale', 'timeZone');

    if (locale && momentObj.locale) {
      momentObj = momentObj.locale(locale);
    }

    if (timeZone && momentObj.tz) {
      momentObj = momentObj.tz(timeZone);
    }

    return momentObj;
  },

I would suggest to change it like this:

  moment() {
    let momentObj;
    let { locale, timeZone } = getProperties(this, 'locale', 'timeZone');

    if (timeZone && moment.tz) {
      momentObj = moment.tz(...arguments, timeZone);
    } else {
      momentObj = moment(...arguments);
    }

    if (locale && momentObj.locale) {
      momentObj = momentObj.locale(locale);
    }

    return momentObj;
  },

The key idea behind the change being: if there's a timeZone property set, and the moment version supports tz, create the moment object using the moment.tz() constructor; else, use the standard moment() constructor.

This change would solve the issue, as explained (hopefully!) in the issue description.

If any of you maintainers guys likes the idea, just drop a line here and I'll try to craft a PR 🙂

[willemdewit]

I agree with @ntgussoni, using the moment function from the service this.get('momentService').moment() gives a different result than from the 'normal' way of making moment objects import moment from 'moment';
My opinion is that the global timezone should be set also to make the behavior the same.

[ntgussoni]

@algodave I get what you are saying now, I agree we should be able to create a moment in a timezone, or we wouldn't be supporting all cases.
However, I also think we should be able to modify the global timezone at the same time you set the default for ember-moment, because it seems odd to set a default and not having it everywhere.

  setTimeZone(timeZone, setGlobal = false) {
    this.changeTimeZone(timeZone, setGlobal);
  },
  changeTimeZone(timeZone, setGlobal = false) {
    if(setGlobal)
       moment.tz.setDefault(timeZone);
    
    set(this, 'timeZone', timeZone);
    this.trigger('timeZoneChanged', timeZone);
  },

No mean to hijack your post with this, but it is related

[gmkohler]

@ntgussoni would implementing what @algodave suggests prevent this in the future?