Skip to content
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

startOf('hour') and endOf('hour') not DST aware #2749

Closed
joelmukuthu opened this issue Nov 13, 2015 · 4 comments · Fixed by #4338
Closed

startOf('hour') and endOf('hour') not DST aware #2749

joelmukuthu opened this issue Nov 13, 2015 · 4 comments · Fixed by #4338

Comments

@joelmukuthu
Copy link

For the hour when DST ended in the ECT (Europe/Copenhagen) timezone, startOf('hour') mutates the moment to the start of the previous hour instead of the current hour:

moment('2015-10-25T02:00:00.000+02:00').unix(); // 1445731200 (an hour before DST ended)
moment('2015-10-25T02:00:00.000+01:00').unix(); // 1445734800 (the extra hour)
moment('2015-10-25T03:00:00.000+01:00').unix(); // 1445738400 (the hour after DST ended)

// however, with startOf('hour'), this happens
moment('2015-10-25T02:00:00.000+02:00').startOf('hour').unix(); // 1445731200 (good)
moment('2015-10-25T02:00:00.000+01:00').startOf('hour').unix(); // 1445731200 (bad, this should be 1445734800)
moment('2015-10-25T03:00:00.000+01:00').startOf('hour').unix(); // 1445738400 (good)

Might be related to #1990. Tested on version 2.10.6

@joelmukuthu
Copy link
Author

As discovered in #1990, this is also a problem in Date.prototype.set{XXX}:

var date = new Date('2015-10-25T02:00:00+01:00');
date.toString(); // Sun Oct 25 2015 02:00:00 GMT+0100 (CET)
date.setMinutes(0);
date.toString(); // Sun Oct 25 2015 02:00:00 GMT+0200 (CEST)

The above happens in Chrome. In Firefox, it happens for the hour before DST ends:

var date = new Date('2015-10-25T02:00:00+02:00');
date.toString(); // Sun Oct 25 2015 02:00:00 GMT+0200 (CEST)
date.setMinutes(0);
date.toString(); // Sun Oct 25 2015 02:00:00 GMT+0100 (CEST)

Both these cases can be avoided by using setTime instead of setMinutes, setSeconds... as described here http://stackoverflow.com/questions/9820468/adding-30-minutes-to-date-causes-it-to-go-back-30-minutes:

var date = new Date('2015-10-25T02:00:00+01:00');
date.toString(); // Sun Oct 25 2015 02:00:00 GMT+0100 (CET)
date.setTime(0); // for startOf('hour')
date.setTime(date.getTime() + (59 * 60000) + (59 * 1000) + 999); // for endOf('hour')
date.toString(); // Sun Oct 25 2015 02:59:59 GMT+0100 (CET)

Could you adopt this implementation for moment?

@icambron
Copy link
Member

Good analysis. I think that would be a reasonable fix, yeah.

@icambron icambron added the Bug label Nov 26, 2015
santigimeno added a commit to santigimeno/cron-parser that referenced this issue Oct 8, 2016
On DST end transition, setting the date to the `startOf` `hour`,
`minute` or `second` after having added 1 `hour`, `minute` or `second`
to the date, would result in the date set to the initial value causing
an infinite loop.

Example:

```js
'use strict';

const moment = require('moment-timezone');

var m = moment(new Date('Sun Oct 30 2016 02:00:00 GMT+0200'));
console.log(m.toString(),
            '<-- Just 1 hour before the DST end @ Europe/Madrid');
m.add(1, 'hours');
console.log(m.toString(),
            '<-- DST end transition @ Europe/Madrid works correctly');
m.startOf('hour');
console.log(m.toString(),
            '<-- Smthing wrong?? It goes back to the previous hour');
```

Fixes: harrisiirak#72
Ref: moment/moment#2749
santigimeno added a commit to santigimeno/cron-parser that referenced this issue Oct 15, 2016
On DST end transition, setting the date to the `startOf` `hour`,
`minute` or `second` after having added 1 `hour`, `minute` or `second`
to the date, would result in the date set to the initial value causing
an infinite loop.

Example:

```js
'use strict';

const moment = require('moment-timezone');

var m = moment(new Date('Sun Oct 30 2016 02:00:00 GMT+0200'));
console.log(m.toString(),
            '<-- Just 1 hour before the DST end @ Europe/Madrid');
m.add(1, 'hours');
console.log(m.toString(),
            '<-- DST end transition @ Europe/Madrid works correctly');
m.startOf('hour');
console.log(m.toString(),
            '<-- Smthing wrong?? It goes back to the previous hour');
```

Fixes: harrisiirak#72
Ref: moment/moment#2749
santigimeno added a commit to santigimeno/cron-parser that referenced this issue Oct 15, 2016
On DST end transition, setting the date to the `startOf` `hour`,
`minute` or `second` after having added 1 `hour`, `minute` or `second`
to the date, would result in the date set to the initial value causing
an infinite loop.

Example:

```js
'use strict';

const moment = require('moment-timezone');

var m = moment(new Date('Sun Oct 30 2016 02:00:00 GMT+0200'));
console.log(m.toString(),
            '<-- Just 1 hour before the DST end @ Europe/Madrid');
m.add(1, 'hours');
console.log(m.toString(),
            '<-- DST end transition @ Europe/Madrid works correctly');
m.startOf('hour');
console.log(m.toString(),
            '<-- Smthing wrong?? It goes back to the previous hour');
```

Fixes: harrisiirak#72
Ref: moment/moment#2749
@marwahaha marwahaha added the DST label Mar 20, 2017
@hifall hifall mentioned this issue Sep 18, 2017
@darakeon
Copy link

@joelmukuthu , maybe it's fixed, endOf for day seems fixed.

@joelmukuthu
Copy link
Author

@darakeon doesn't seem fixed yet, I just tried with v2.20.1. I think it will be fixed by this PR though: #4338

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

Successfully merging a pull request may close this issue.

4 participants