Fix handling of DST jumps in Brazilian timezones in startOf/endOf

[Seldaek]

Fixes #3132

The approach taken here is a bit different than previous ideas (e.g. #3716) where startOf was called twice in endOf. Here we apply endOf much like startOf is done, so that startOf does not have to be called in endOf. Then in startOf it also checks for DST failures where some browsers return the previous or next day when they are set to a midnight that does not exist due to the DST jump. This is handled only for those time units which require it to avoid performance issues, but overall the performance impact should be very low as it only does a couple extra comparison in most cases.

The patch is demonstrated in https://jsfiddle.net/1gpr1fys/7/ in case you want to play with it and do your own tests. It logs all true in the console if you have the correct timezone (Brazil/Sao Paulo or Brasilia). Works in both Firefox and Chrome.

If you have another timezone that does the DST switch not at midnight, then tests 1 and 4 in the demo fail because they return midnight instead of 1am, that's normal. The test suite of moment though passes as it always mimicks the Brazil timezone in the tests checking for this behavior.

[icambron]

Tagging @mj1856 as the most likely person to be able to shepherd this along.

[ichernev]

@Seldaek can you explain which case are you fixing (not "brazillian" but put more technical perspective). Also I'm a bit against doing twice the work in endOf. Can you elaborate more on why it is needed.

[Seldaek]

@ichernev The case is that some timezones (Brazil/Sao_Paolo is one) switch DST at midnight, which means that midnight does not exist but it switches from 23:59:59 to 01:00:00 on the DST switch date. Due to this, and some different ways Firefox and Chrome handle dates, momentjs ends up jumping to the wrong date when using startOf/endOf. The problem is also present when doing things like adding an hour if you are at 23:00:00 of course, but that's less common and way harder to fix, so it is not in the scope here.

I don't know exactly what else you want, the PR description and jsfiddle have a lot of info already.

[darakeon]

I made a PR too, didn't saw this one. The one I did solves just the problem of daylight saving time.
#4254

[darakeon]

Seems like the problem is fixed in current version. @Seldaek

[lourenci]

This problem still persists in current version.

[Seldaek]

@ichernev anything I can do to help get this in? We've been running with it in production since september without any issues.

[coveralls]

Coverage increased (+0.05%) to 94.7% when pulling b359ec2 on Seldaek:GH3132 into 2e2a5b3 on moment:develop.

[marwahaha]

@Seldaek - Thanks for this. Sorry it never got merged. Can you check if #4338 will solve your issue?

[Seldaek]

@marwahaha I realistically won't have the time to check for this next few weeks so I'd say if you think it's good go ahead and merge it, once it's in a release I will try and migrate our codebase back to mainline moment see if things still work

[lourenci]

@marwahaha #4338 doesn't fix that.

2.22.2

#4338

This pull seems to not solve too:

[Seldaek]

With our code (https://github.com/Seldaek/moment/commits/integration) I get this:

With CET timezone:

moment('2018-11-04').endOf('day').format()
"2018-11-04T23:59:59+01:00"

Then I switched to Brasilia timezone:

moment('2018-11-04').endOf('day').format()
"2018-11-04T23:59:59-02:00"

Seems fixed to me..

[lourenci]

@Seldaek the integration branch is ok. The GH3132 is not.

edit: misspelling

[Seldaek]

Did you build momentjs though? If you just check out the branch without building then you're not getting any of the fixes.

[lourenci]

I don't. Sorry. I just pointed my package.json to the git branch.

[lourenci]

@marwahaha I'm sorry for that. As the @Seldaek has mentioned, I forgot the build it.

The #4338 solves this issue.

[marwahaha]

closing in favor of #4338