Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

amMomentConfig.timezone has no effect #271

Open
CyborgMaster opened this issue Oct 18, 2016 · 17 comments
Open

amMomentConfig.timezone has no effect #271

CyborgMaster opened this issue Oct 18, 2016 · 17 comments

Comments

@CyborgMaster
Copy link

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
Copy link
Owner

urish commented Oct 18, 2016

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

@CyborgMaster
Copy link
Author

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
Copy link
Owner

urish commented Oct 18, 2016

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

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
Copy link
Author

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

@urish
Copy link
Owner

urish commented Oct 18, 2016

Thank you!

@CyborgMaster
Copy link
Author

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
Copy link
Owner

urish commented Oct 18, 2016

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

@CyborgMaster
Copy link
Author

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
Copy link
Owner

urish commented Oct 18, 2016

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
Copy link
Author

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
Copy link
Owner

urish commented Oct 18, 2016

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
Copy link
Author

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
Copy link
Owner

urish commented Oct 19, 2016

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
Copy link
Author

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
Copy link

Does the problem is solved @CyborgMaster ?

@CyborgMaster
Copy link
Author

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

@SubJunk
Copy link

SubJunk commented Feb 10, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants