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
[bugfix] Fix startOf/endOf DST issues while boosting performance #4338
Merged
Merged
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 321a7ce
Performance improvements
ashsearle b3f5e3b
Rewrite for legibility
ashsearle c447c0c
Fix for years in range 0-99
ashsearle 69ccb83
Remove placeholder comments
ashsearle 1cdb7ca
Revert comment
ashsearle 09203bb
Remove useless special-case
ashsearle 119e0b8
Code review change: use break consistenly
ashsearle 1b433f7
Rename per code review
ashsearle 657db78
Fix typo
ashsearle e2c82e1
Inline variable to end naming debate
ashsearle 6abd6fd
Address review comments
ashsearle File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.