urish / angular-moment

Moment.JS directives for Angular.JS (timeago and more)
MIT License
2.6k stars 397 forks source link

`amMomentConfig.timezone` has no effect #271

Open CyborgMaster opened 7 years ago

CyborgMaster commented 7 years ago

I just upgraded to 1.0.0 and amMomentConfig.timezone doesn't seem to do anything anymore.

It used to be that the following would always display in the set timezone:

<td>{{ appointment.startsAt | amDateFormat: 'l LT' }}</td>

Now it appears that amDateFormat doesn't apply any timezone shifting. Am I missing something?

amDateFormat used to run the following in version 0.10.3

return amMoment.applyTimezone(date, timezone).format(format);

now it skips applyTimezone which is what converted it to the default timezone and simple returns this:

return date.format(format);

Is there some new way to set up the default timezone, or make sure it gets applied correctly?

urish commented 7 years ago

It should be handled by preprocessDate(), which is called by amDateFormat

CyborgMaster commented 7 years ago

Sounds good, but preprocessDate doesn't seem to do that. Here's it's full code:

                this.preprocessDate = function (value) {
                    // Configure the default timezone if needed
                    if (defaultTimezone !== angularMomentConfig.timezone) {
                        this.changeTimezone(angularMomentConfig.timezone);
                    }

                    if (angularMomentConfig.preprocess) {
                        return angularMomentConfig.preprocess(value);
                    }

                    if (!isNaN(parseFloat(value)) && isFinite(value)) {
                        // Milliseconds since the epoch
                        return moment(parseInt(value, 10));
                    }

                    // else just returns the value as-is.
                    return moment(value);
                };

There is a call to changeTimezone in there, but that just sets up global configs, it doesn't actually modify the moment object in question.

For contrast, here's how applyTimezone did it before:

                this.applyTimezone = function (aMoment, timezone) {
                    timezone = timezone || angularMomentConfig.timezone;
                    if (!timezone) {
                        return aMoment;
                    }

                    if (timezone.match(/^Z|[+-]\d\d:?\d\d$/i)) {
                        aMoment = aMoment.utcOffset(timezone);
                    } else if (aMoment.tz) {
                        aMoment = aMoment.tz(timezone);
                    } else {
                        $log.warn('angular-moment: named timezone specified but moment.tz() is undefined. Did you forget to include moment-timezone.js?');
                    }

                    return aMoment;
                };

You can clearly see the default timezone retrieved at the top, and then the call to .tz later in the function to apply the timezone.

All of my dates in my app are displaying in UTC instead of the configured timezone. Am I missing something?

urish commented 7 years ago

Sounds like it could be a regression. The change was introduced in commit 4f3010b34058d90a093137e5a9b359e1a140670c.

Any chance you can create a regression test case (a unit test case that passed in 0.10.3, but fails with 1.0.0) ?

CyborgMaster commented 7 years ago

I can sure try. Let's see what I can come up with.

urish commented 7 years ago

Thank you!

CyborgMaster commented 7 years ago

Here's a failing test. It passes on 0.10.3

        it('should apply the configured timezone to moment objects', function () {
            angularMomentConfig.timezone = 'Pacific/Tahiti';
            var timestamp = moment.tz('2012-01-22T12:46:54Z', 'UTC');
            expect(amDateFormat(timestamp, '(HH,mm,ss);MM.DD.YYYY')).toBe('(02,46,54);01.22.2012');
        });

It's just like this one, except it uses a moment object instead of a JS Date.

        it('should respect the configured timezone', function () {
            angularMomentConfig.timezone = 'Pacific/Tahiti';
            var timestamp = Date.UTC(2012, 0, 22, 12, 46, 54);
            expect(amDateFormat(timestamp, '(HH,mm,ss);MM.DD.YYYY')).toBe('(02,46,54);01.22.2012');
        });
urish commented 7 years ago

Spot on! Any chance you can create a PR with this test case and the fix?

CyborgMaster commented 7 years ago

I'm not sure I have the background to do so. In 0.10.3, there is the applyTimezone function that is used all over the place. In 1.0.0 it has been removed. I'm not sure where the functionality is suppose to be removed due to the new amTimezone filter and what are regressions. You might know better than I.

urish commented 7 years ago

Actually, it is a little tricky.

In 0.10.3, amDateFormat had a parameter for the timezone, and if this parameter wasn't provided, it used the default timezone. As of 1.0.0, there is a new amTimezone filter that is used to set the timezone. So if amDateFormat would automatically apply the default timezone (in preprocessDate), it would override any timezone you may have set with amTimezone.

I believe that the best solution would just be to stop using amMomentConfig.timezone and set the default timezone through moment yourself, before you create any moment objects.

CyborgMaster commented 7 years ago

Ahh, see that's not that simple for me either. All of my dates come down from the server in UTC so are parsed in moment as having a UTC timezone. I want them to stay in UTC while doing processing, but I was using angular-moment to make sure they were always converted to the user's timezone before display.

I understand your reasoning, but this was my main reason for including angular-moment. If angular-moment isn't going to do this moving forward, can you think of another way to make sure to convert the timezone on display? (Also, you might want to remove the config option if it doesn't really do anything).

urish commented 7 years ago

Hmm... Depending on what you actually do when processing, the timezone may not have any effect there anyway.

But if you wanted to achieve a pre-1.0.0 behavior, and don't need to override the timezone in specific cases with amTimezone, I believe the easiest way would be to declare a preprocess function as follows:

angularMomentConfig.preprocess = function(value) {
    if (!isNaN(parseFloat(value)) && isFinite(value)) {
       return moment(parseInt(value, 10));
    }
    return moment(value).tz('Desired timezone');
};
CyborgMaster commented 7 years ago

Not a bad idea. I'm signing off of work for today, so I'll check on this tomorrow. It might be the right thing for my project.

You still might want to consider removing the timezone option if it no longer affects amDateFormat, which is what the readme says it does now:

Time zone support

The amDateFormat and amCalendar filters can be configured to display dates aligned to a specific timezone. You can configure the timezone using the following syntax:

The other thing you might be able to do is hang an extra attribute on the moment object (ex: amMomentTimezoneOverriden) when it is set by amTimezone and then use that to determine if amDateFormat should use the default. I don't know if a custom property would survive other processing though.

urish commented 7 years ago

Thanks for the suggestions, I think updating the docs to reflect this would make more sense. Hanging an extra attribute on the moment object sounds like "too much magic" for me, and there will probably be some corner cases, etc.

CyborgMaster commented 7 years ago

I agree that splitting things up into multiple compossible filters made the project less complicated and more approachable, but it does seem like we've lost some important functionality here. It used to be that you could set a default display timezone for your whole project, and then it would be used, unless you specifically overrode it in amDateFormat, but now it seems that there is no way to do that. I can add a preprocessor like you suggested, but that prevents me from overriding the timezone in specific instances because it forced the timezone every time, or I can use the amTimezone filter, but that requires the timezone be specified every time.

A related question: If the timezone option in angularMomentConfig doesn't set the default display timezone, does it have any other purpose? Should the entire config option be removed.

ptitdje45 commented 7 years ago

Does the problem is solved @CyborgMaster ?

CyborgMaster commented 7 years ago

Not really. I had to use the preprocess function suggested by @urish, so I'm hard coded to one timezone per browser instance.

SubJunk commented 7 years ago

I just ran into this issue as well. It's a shame that the constant is no longer working. +1