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

When year is in some far future - use offsets of some similar year before 2038 #881

Closed
wants to merge 1 commit into from
Closed

When year is in some far future - use offsets of some similar year before 2038 #881

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 30 additions & 0 deletions moment-timezone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,11 @@
return i;
}
}

var alternative = this._getAlternative(timestamp);
Copy link

Choose a reason for hiding this comment

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

I don't think it makes sense for this logic to be in _index, since it's not returning the index for an until that represents the timestamp. It would have the correct offset. I think it would be better to create a different method that wraps this information instead, and have the other method call the existing _index.

if (alternative) {
return this._index(alternative);
}
},

countries : function () {
Expand Down Expand Up @@ -200,9 +205,24 @@
return offsets[i];
}
}

var alternative = this._getAlternative(timestamp);
if (alternative) {
return this.offset(alternative);
}

return offsets[max];
},

_getAlternative: function (timestamp) {
var similarYear = findYearLike(new Date(timestamp).getFullYear());

if (similarYear) {
var result = new Date(timestamp);
result.setFullYear(similarYear);
return result.getTime();
}
},

abbr : function (mom) {
return this.abbrs[this._index(mom)];
Expand Down Expand Up @@ -330,6 +350,16 @@
}
}

function findYearLike(year) {
for (var j = 2038; j > 2000; j--) {
Copy link

Choose a reason for hiding this comment

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

If we aren't worried about strange exceptions (like when a timezone skips a day entirely), we use the fact that there's a 28-year cycle in which the calendar repeats itself, instead of performing a loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I already fixed that, will push tomorrow, it was beta version to check if that logics (of searching similar years) will work at all

if (new Date(year + '-01-01').getDay() === new Date(j + '-12-31').getDay() &&
Copy link

Choose a reason for hiding this comment

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

You're repeating the same check here instead of comparing the first day of the week of the target year with the last day of the week of the candidate year, instead of comparing the first and last days of the week of the target year with those of the candidate year.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed that already, will push changes tomorrow

Copy link

Choose a reason for hiding this comment

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

Any news about the fix @ellenaua ? :)

new Date(year + '-01-01').getDay() === new Date(j + '-12-31').getDay()
) {
return j;
}
}
}

function guessesForUserOffsets (offsets) {
var offsetsLength = offsets.length,
filteredGuesses = {},
Expand Down