New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using Z suffix when in UTC mode (#3020) #3098

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
7 participants
@lcarva
Contributor

lcarva commented Apr 8, 2016

As described in issue #3020, a moment in UTC mode
should be formatted, by default, with the letter Z
as a suffix instead of the time zone, +00:00.

Using Z suffix when in UTC mode (#3020)
As described in issue #3020, a moment in UTC mode
should be formatted, by default, with the letter Z
as a suffix instead of the time zone, +00:00.
@icambron

This comment has been minimized.

Member

icambron commented Apr 8, 2016

+1 on the implementation. The only other possibility to consider is whether it makes sense to have a Z-like token that formats "Z" or a numerical offset based on whether it's UTC or not and then use that token in the ISO format; i.e. move the if (isUTC() code out of the ISO implementation and into the (or rather a) token implementation.

But I think it's probably fine as-is.

@lcarva

This comment has been minimized.

Contributor

lcarva commented Apr 8, 2016

@icambron I like the idea of using a token since it's more generic and would work with any kind of formatting, not just the default. I think this can be done by editing

addFormatToken(token, 0, 0, function () {
to simply return the letter Z instead of the time offset. Thoughts?

@maggiepint

This comment has been minimized.

Member

maggiepint commented Apr 8, 2016

Just a heads up on this PR. I think if this gets merged, it's going to exacerbate the effect of moment/moment-timezone#312 . Dates in London time, without BST, are going to start being formatted with an offset of Z instead of (the I think more correct) +00:00.
It's not a problem with this PR, it's a problem with the way moment timezone and moment interact. But in any case, I wanted to point out that this will happen.

@mj1856

This comment has been minimized.

Member

mj1856 commented Apr 8, 2016

@maggiepint - right. let's fix it in moment proper, then address the moment-timezone side. Even if times in London are temporarily formatted with a Z, that won't break anything much. Yes, those should be +00:00, but like I said, it's a semantic difference only.

Also - I don't think it matters to us, but worth mentioning that RFC3339 allows -00:00 also, which is supposed to mean that the time isn't UTC, and the original offset isn't +00:00 either, but you've converted to UTC and lost track of the original offset. 😕

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 12, 2016

After -00:00 we just need a method that finds the lost offset a-la https://xkcd.com/416/ ;-)
Lets schedule this for next release.

@ichernev ichernev added this to the 2.13.0 milestone Apr 12, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 16, 2016

Merged in 5a93650

@leolannenmaki

This comment has been minimized.

leolannenmaki commented May 19, 2016

We actually got bitten by a bug caused by this change in production :(

The quick fix was:

-    "moment": "^2.10.6",
+    "moment": "~2.10.6",

We send a date to an external API using moment.utc().format(). We deployed the service again and because of the version range that we allowed in our package.json the version of moment jumped to 2.13.0.

The external API parsed the format 2016-05-18T19:20:27+00:00 fine but didn't manage to parse 2016-05-18T19:20:03Z and that caused issues.

@mj1856

This comment has been minimized.

Member

mj1856 commented May 24, 2016

@leolannenmaki - Thanks for the feedback, and sorry to hear this caused a production bug for you.

However, I'd suggest that since both +00:00 and Z are valid representations by ISO8601 and RFC3339, and Z is extremely common (such as with the JS Date.toISOString function and others), that you should consider the external API to have a bug if it can't accept Z.

@leolannenmaki

This comment has been minimized.

leolannenmaki commented May 25, 2016

No worries :)

I completely agree that the external API has a bug in this case. I think it's using some old PHP lib to parse the date. The problem is just that we don't have control over it. I'd just like you guys to consider in the future that these kind of bugfixes / conforming to the spec can also be backward incompatible major changes in a sense. My message was more of a heads up that this can be an issue to some users.

@paulfalgout

This comment has been minimized.

paulfalgout commented Sep 7, 2017

I also got bitten in production by this update as utc dates no longer adhere to moment.defaultFormat So in modifying the defaultFormat, simply because I needed to send an oddly formatted date in UTC my code broke.
I think this is probably a good update, but it was certainly breaking.
However the bigger issue I think is that this new functionality seems to be undocumented. So modifying defaultFormat appears to have no effect and according to the documentation it is the old interface.

@mj1856

This comment has been minimized.

Member

mj1856 commented Sep 8, 2017

@paulfalgout - sorry for the trouble. Sometimes the line between "bug fix" and "breaking change" is fuzzy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment