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.

Sign up for GitHub

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

Jump to bottom

[bugfix] Fix startOf/endOf DST issues while boosting performance #4338

Merged
merged 12 commits into from Jan 19, 2019
Merged

[bugfix] Fix startOf/endOf DST issues while boosting performance #4338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
caa8f5a
Fix startOf/endOf DST issues
ashsearle Nov 29, 2017
321a7ce
Performance improvements
ashsearle Nov 29, 2017
b3f5e3b
Rewrite for legibility
ashsearle Nov 30, 2017
c447c0c
Fix for years in range 0-99
ashsearle Nov 30, 2017
69ccb83
Remove placeholder comments
ashsearle Nov 30, 2017
1cdb7ca
Revert comment
ashsearle Dec 1, 2017
09203bb
Remove useless special-case
ashsearle Dec 6, 2017
119e0b8
Code review change: use break consistenly
ashsearle Dec 22, 2017
1b433f7
Rename per code review
ashsearle Jan 5, 2018
657db78
Fix typo
ashsearle Apr 16, 2018
e2c82e1
Inline variable to end naming debate
ashsearle Apr 16, 2018
6abd6fd
Address review comments
ashsearle Jan 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 22 additions & 8 deletions src/lib/create/date-from-array.js
View file
Open in desktop
@@ -1,21 +1,35 @@
export function createDate (y, m, d, h, M, s, ms) {
// can't just apply() to create a date:
// https://stackoverflow.com/q/181348
var date = new Date(y, m, d, h, M, s, ms);

var date;
// the date constructor remaps years 0-99 to 1900-1999
if (y < 100 && y >= 0 && isFinite(date.getFullYear())) {
date.setFullYear(y);
if (y < 100 && y >= 0) {
// preserve leap years using a full 400 year cycle, then reset
date = new Date(y + 400, m, d, h, M, s, ms);
if (isFinite(date.getFullYear())) {
date.setFullYear(y);
}
} else {
date = new Date(y, m, d, h, M, s, ms);
}

return date;
}

export function createUTCDate (y) {
var date = new Date(Date.UTC.apply(null, arguments));

var date;
// the Date.UTC function remaps years 0-99 to 1900-1999
if (y < 100 && y >= 0 && isFinite(date.getUTCFullYear())) {
date.setUTCFullYear(y);
if (y < 100 && y >= 0) {
var args = Array.prototype.slice.call(arguments);
// preserve leap years using a full 400 year cycle, then reset
args[0] = y + 400;
date = new Date(Date.UTC.apply(null, args));
if (isFinite(date.getUTCFullYear())) {
date.setUTCFullYear(y);
}
} else {
date = new Date(Date.UTC.apply(null, arguments));
}

return date;
}
131 changes: 100 additions & 31 deletions src/lib/moment/start-end-of.js
View file
Open in desktop
@@ -1,59 +1,128 @@
import { normalizeUnits } from '../units/aliases';
import { hooks } from '../utils/hooks';

var MS_PER_SECOND = 1000;
var MS_PER_MINUTE = 60 * MS_PER_SECOND;
var MS_PER_HOUR = 60 * MS_PER_MINUTE;
var MS_PER_400_YEARS = (365 * 400 + 97) * 24 * MS_PER_HOUR;

// actual modulo - handles negative numbers (for dates before 1970):
function mod(dividend, divisor) {
return (dividend % divisor + divisor) % divisor;
}

function localStartOfDate(y, m, d) {
// the date constructor remaps years 0-99 to 1900-1999
if (y < 100 && y >= 0) {
// preserve leap years using a full 400 year cycle, then reset
return new Date(y + 400, m, d) - MS_PER_400_YEARS;
} else {
return new Date(y, m, d).valueOf();
}
}

function utcStartOfDate(y, m, d) {
// Date.UTC remaps years 0-99 to 1900-1999
if (y < 100 && y >= 0) {
// preserve leap years using a full 400 year cycle, then reset
return Date.UTC(y + 400, m, d) - MS_PER_400_YEARS;
} else {
return Date.UTC(y, m, d);
}
}

export function startOf (units) {
var time;
units = normalizeUnits(units);
// the following switch intentionally omits break keywords
// to utilize falling through the cases.
if (units === undefined || units === 'millisecond' || !this.isValid()) {
return this;
}

var startOfDate = this._isUTC ? utcStartOfDate : localStartOfDate;

switch (units) {
case 'year':
this.month(0);
/* falls through */
time = startOfDate(this.year(), 0, 1);
break;
case 'quarter':
time = startOfDate(this.year(), this.month() - this.month() % 3, 1);
break;
case 'month':
this.date(1);
/* falls through */
time = startOfDate(this.year(), this.month(), 1);
break;
case 'week':
time = startOfDate(this.year(), this.month(), this.date() - this.weekday());
break;
case 'isoWeek':
time = startOfDate(this.year(), this.month(), this.date() - (this.isoWeekday() - 1));
break;
case 'day':
case 'date':
this.hours(0);
/* falls through */
time = startOfDate(this.year(), this.month(), this.date());
break;
case 'hour':
this.minutes(0);
/* falls through */
time = this._d.valueOf();
time -= mod(time + (this._isUTC ? 0 : this.utcOffset() * MS_PER_MINUTE), MS_PER_HOUR);
break;
case 'minute':
this.seconds(0);
/* falls through */
time = this._d.valueOf();
time -= mod(time, MS_PER_MINUTE);
break;
case 'second':
this.milliseconds(0);
}

// weeks are a special case
if (units === 'week') {
this.weekday(0);
}
if (units === 'isoWeek') {
this.isoWeekday(1);
}

// quarters are also special
if (units === 'quarter') {
this.month(Math.floor(this.month() / 3) * 3);
time = this._d.valueOf();
time -= mod(time, MS_PER_SECOND);
break;
}

this._d.setTime(time);
hooks.updateOffset(this, true);
return this;
}

export function endOf (units) {
var time;
units = normalizeUnits(units);
if (units === undefined || units === 'millisecond') {
if (units === undefined || units === 'millisecond' || !this.isValid()) {
return this;
}

// 'date' is an alias for 'day', so it should be considered as such.
if (units === 'date') {
units = 'day';
var startOfDate = this._isUTC ? utcStartOfDate : localStartOfDate;

switch (units) {
case 'year':
time = startOfDate(this.year() + 1, 0, 1) - 1;
break;
case 'quarter':
time = startOfDate(this.year(), this.month() - this.month() % 3 + 3, 1) - 1;
break;
case 'month':
time = startOfDate(this.year(), this.month() + 1, 1) - 1;
break;
case 'week':
time = startOfDate(this.year(), this.month(), this.date() - this.weekday() + 7) - 1;
break;
case 'isoWeek':
time = startOfDate(this.year(), this.month(), this.date() - (this.isoWeekday() - 1) + 7) - 1;
break;
case 'day':
case 'date':
time = startOfDate(this.year(), this.month(), this.date() + 1) - 1;
break;
case 'hour':
time = this._d.valueOf();
time += MS_PER_HOUR - mod(time + (this._isUTC ? 0 : this.utcOffset() * MS_PER_MINUTE), MS_PER_HOUR) - 1;
break;
case 'minute':
time = this._d.valueOf();
time += MS_PER_MINUTE - mod(time, MS_PER_MINUTE) - 1;
break;
case 'second':
time = this._d.valueOf();
time += MS_PER_SECOND - mod(time, MS_PER_SECOND) - 1;
break;
}

return this.startOf(units).add(1, (units === 'isoWeek' ? 'week' : units)).subtract(1, 'ms');
this._d.setTime(time);
hooks.updateOffset(this, true);
return this;
}
24 changes: 24 additions & 0 deletions src/test/moment/start_end_of.js
View file
Open in desktop
Expand Up @@ -393,3 +393,27 @@ test('endOf millisecond and no-arg', function (assert) {
assert.equal(+m, +m.clone().endOf('millisecond'), 'endOf with millisecond argument should change time');
assert.equal(+m, +m.clone().endOf('milliseconds'), 'endOf with milliseconds argument should change time');
});

test('startOf for year zero', function (assert) {
var m = moment('0000-02-29T12:34:56.789Z').parseZone();
assert.equal(m.clone().startOf('ms').toISOString(), '0000-02-29T12:34:56.789Z', 'startOf millisecond should preserve year');
assert.equal(m.clone().startOf('s').toISOString(), '0000-02-29T12:34:56.000Z', 'startOf second should preserve year');
assert.equal(m.clone().startOf('m').toISOString(), '0000-02-29T12:34:00.000Z', 'startOf minute should preserve year');
assert.equal(m.clone().startOf('h').toISOString(), '0000-02-29T12:00:00.000Z', 'startOf hour should preserve year');
assert.equal(m.clone().startOf('d').toISOString(), '0000-02-29T00:00:00.000Z', 'startOf day should preserve year');
assert.equal(m.clone().startOf('M').toISOString(), '0000-02-01T00:00:00.000Z', 'startOf month should preserve year');
assert.equal(m.clone().startOf('Q').toISOString(), '0000-01-01T00:00:00.000Z', 'startOf quarter should preserve year');
assert.equal(m.clone().startOf('y').toISOString(), '0000-01-01T00:00:00.000Z', 'startOf year should preserve year');
});

test('endOf for year zero', function (assert) {
var m = moment('0000-02-29T12:34:56.789Z').parseZone();
assert.equal(m.clone().endOf('ms').toISOString(), '0000-02-29T12:34:56.789Z', 'endOf millisecond should preserve year');
assert.equal(m.clone().endOf('s').toISOString(), '0000-02-29T12:34:56.999Z', 'endOf second should preserve year');
assert.equal(m.clone().endOf('m').toISOString(), '0000-02-29T12:34:59.999Z', 'endOf minute should preserve year');
assert.equal(m.clone().endOf('h').toISOString(), '0000-02-29T12:59:59.999Z', 'endOf hour should preserve year');
assert.equal(m.clone().endOf('d').toISOString(), '0000-02-29T23:59:59.999Z', 'endOf day should preserve year');
assert.equal(m.clone().endOf('M').toISOString(), '0000-02-29T23:59:59.999Z', 'endOf month should preserve year');
assert.equal(m.clone().endOf('Q').toISOString(), '0000-03-31T23:59:59.999Z', 'endOf quarter should preserve year');
assert.equal(m.clone().endOf('y').toISOString(), '0000-12-31T23:59:59.999Z', 'endOf year should preserve year');
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some more tests about the issues you are referencing.

For example, I'm not convinced this PR will solve #1990
There are examples in #2749, #3580, and in #3132, #4152.

We should be able to construct test in particular locales. I'm not sure about testing in conjunction with moment-tz, but let me know how far you get.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to add tests that'll run on Travis for some of these things...

(Issue #952 addresses the difficulty of this and talks about a brute-force approach running tests across all timezones using a grunt zones task - but that task doesn't exist...)

When I was working on tests, I'd try and identify a specific date and timezone showing the problem, and run eye-ball a test run from a bash script. Something like this:

#!/bin/bash
moment=./build/umd/moment.js

echo "Issue 2749:"
TZ='Europe/Copenhagen' node -e "$(cat << EOF
var moment = require('${moment}');
console.log("Expected:", moment('2015-10-25T02:00:00.000+01:00').unix());
console.log("Actual:  ", moment('2015-10-25T02:00:00.000+01:00').startOf('hour').unix());
EOF
)"

echo "Issue 3132:"
TZ='America/Sao_Paulo' node -e "$(cat << EOF
var moment = require('${moment}');
// Bug: Brazil was showing wrong end-of-day for 2016-10-16:
console.log("Expected: 2016-10-16 23:59:59");
console.log("Actual:   " + moment("2016-10-16").endOf('day').format("YYYY-MM-DD HH:mm:ss"));

// Check start of day:
console.log("Expected: 2016-10-16 01:00:00");
console.log("Actual:   " + moment("2016-10-16T12:34").startOf('day').format("YYYY-MM-DD HH:mm:ss"));
EOF
)"

Talking about #1990 specifically though: that one is fixed because the problem is in browser variation in how setMinutes/Seconds/Milliseconds is implemented (e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1084547) - by avoiding these methods, we avoid the bug.