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

Modifies all dates in the calendar to have their time set to noon #114

Merged
merged 1 commit into from
Oct 12, 2016

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Oct 11, 2016

This is a fix for #110

I also double-checked this change in the Brazilian time zone and noted that there is no reoccurrence of the DST bug that caused there to be an extra day in the month of October. It might be good to double-check Australia as well.

I'm not like incredibly happy about this, but it seems to work. :|

to: @timReynolds @ljharb
FYI: @airbnb/webinfra @wmartins

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM, but, tests? ༼ つ ◕_◕ ༽つ

@ljharb ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Oct 11, 2016
@majapw
Copy link
Collaborator Author

majapw commented Oct 11, 2016

WAIT I GET IT.

So the general idea is that we were setting the utc offset like so:
month.clone().utcOffset(month.utcOffset());
month is generally set to today's date plus/minus a number of month, courtesy of moment's api. So actually, where this happens (whether it's the Feb/Mar border like in 2018 today or the Mar/Apr border like in 2017 today) depends on which side of the DST day in March we're on). We then set the offset for the entire month based on this initial day and the offset for the entire month after that similarly. As a result, the confusion (one day equaling another due to DST) does not happen on the DST change date itself but on the edge of that month. It also only happens once because the problem only exists when the offset from one day is different from the next. And uh, yeah. THAT'S ALL I'VE GOT.

I am mostly writing this down so that years down the line we'll have a record of this.

@majapw majapw force-pushed the maja-change-all-dates-to-have-noon-as-their-time branch from 39b2b92 to 6b8cb0b Compare October 11, 2016 22:58
@majapw
Copy link
Collaborator Author

majapw commented Oct 11, 2016

just kidding, thanks for making me write tests @ljharb. The original code didn't actually do what I expected. :) This should fix things. PTAL!

@@ -20,6 +20,10 @@ describe('toMomentObject', () => {
expect(toMomentObject()).to.equal(null);
});

it('output has time of 12PM', () => {
Copy link
Member

Choose a reason for hiding this comment

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

it might be nice to test the actual conditions - like, that the thing resulting in the extra day being highlighted returns the correct result

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This becomes a bit harder to test I think because it... depends on a lot of different factors. I mean, I guess we can check that the first of april/last of march and first of march/last of april do not make isSameDay true but that seems like a bad test?

Copy link
Member

Choose a reason for hiding this comment

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

That would test the actual bug this had reported, and assuming the dates were somewhat hardcoded, would likely prevent a regression. Changing the times to 12:01, for example, shouldn't fail tests, because it wouldn't trigger the underlying bug.

@wmartins
Copy link
Contributor

wmartins commented Oct 12, 2016

Hey @majapw, I was just fiddling with the code here and I think I've found a simple (and reliable) solution for this problem.

First of all, the utcOffset fix was done to fix the strange behavior of moment#add, that was adding an extra day when there is a timezone change (i.e.: Brazil entering in daylight saving time). The problem is that this utc date is being propagated to more parts of the library, and this is causing the problem, because every other method is now using this "mutated" object (causing the problem with isSameDay as reported by @timReynolds).

So, I think that if you do the reverse process when using the day, you'll be "safe".

In this case, there are just three simple changes.

The first is in prevDay:

// old
const prevDay = enableOutsideDays && currentDay.clone().subtract(i + 1, 'day');
// new
const prevDay = enableOutsideDays && currentDay.clone().subtract(i + 1, 'day').local();

The second is in currentWeek.push

// old
currentWeek.push(currentDay.clone());
// new
currentWeek.push(currentDay.clone().local());

And the third in nextDay:

// old
const nextDay = enableOutsideDays && currentDay.clone().add(count, 'day');
// new
const nextDay = enableOutsideDays && currentDay.clone().add(count, 'day').local();

What do you think about that @majapw?


More about moment#local.
(Updating:) - All tests passed with this solution

@majapw
Copy link
Collaborator Author

majapw commented Oct 12, 2016

Does that actually still solve the DST issue @wmartins? I kind of feel like the setting the time to the middle of the day is still a bit safer and a bit simpler.

@wmartins
Copy link
Contributor

I would need to test a little bit more to confirm more cases, but if you
think that it is more simple to set the hours, I think it is not a problem.

Just investigated the issue a little bit and brought this alternative
solution. But I think your solution is fine!

Em qua, 12 de out de 2016 12:28, Maja Wichrowska notifications@github.com
escreveu:

Does that actually still solve the DST issue @wmartins
https://github.com/wmartins? I kind of feel like the setting the time
to the middle of the day is still a bit safer and a bit simpler.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#114 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABfQ4Tq_5U3vGsCkaXt5m_VPnQoXOnnAks5qzPyggaJpZM4KULYI
.

@majapw majapw force-pushed the maja-change-all-dates-to-have-noon-as-their-time branch 2 times, most recently from 5a24720 to 387e62c Compare October 12, 2016 17:37
@majapw
Copy link
Collaborator Author

majapw commented Oct 12, 2016

@ljharb I added some tests that I confirmed used to fail (with the utcOffset) and now pass. Can you PTAL and then we can get this bad boy merged in. :)

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM! Only nonblocker comments.

describe('Daylight Savings Time issues', () => {
it('last of February does not equal first of March', () => {
const february = getCalendarMonthWeeks(today.clone().month(1));
const lastWeekOfFebruary = february[february.length - 1].filter(day => !!day);
Copy link
Member

Choose a reason for hiding this comment

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

fwiw .filter(Boolean) works here too :-)

const lastOfFebruary = lastWeekOfFebruary[lastWeekOfFebruary.length - 1];

const march = getCalendarMonthWeeks(today.clone().month(2));
const firstOfMarch = march[0].filter(day => !!day)[0];
Copy link
Member

Choose a reason for hiding this comment

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

This should probably use .find(Boolean) instead

@majapw majapw force-pushed the maja-change-all-dates-to-have-noon-as-their-time branch from 387e62c to 1f1c5d0 Compare October 12, 2016 18:20
@majapw
Copy link
Collaborator Author

majapw commented Oct 12, 2016

Awesome! I fixed the tests that were breaking as a result of changing the date's time. :) So we should be set now. Will merge in after tests pass.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.969% when pulling 1f1c5d0 on maja-change-all-dates-to-have-noon-as-their-time into 73b3ff6 on master.

@majapw majapw merged commit 7b58a2e into master Oct 12, 2016
@majapw majapw deleted the maja-change-all-dates-to-have-noon-as-their-time branch October 12, 2016 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants