Fixes repeated dates - issue #1098 #1101
Conversation
|
@jbruni @bekos is there a way to write a test for this? I know that testing date related stuff is super-tricky so we might want to skip a test for this change, but if we can test it somehow it would be awesome. @bekos feel free to merge if this looks good to you (or I can merge it after your thumbs up). |
|
What about something like this, using moment.js: $.each([30, 60, 90, 120, ... /* each timezone in 30 minute intervals */], function(_, zone) {
describe('timezone ' + zone, function() {
// Test each month of the year
$.each([0,1,2,3,4,5,6,7,8,9,10,11], function(_, month) {
it('month should not contain repeated dates', function() {
// Set $scope.date to the first date of this month, generating the date with moment(...).zone(zone).toDate()
// Walk the table to make sure that from the start of the month until the end of the month, no dates are
// repeated.
//
// Maybe do this for each date in the month too, for extra robustness?
});
});
});
});It would generate a lot of tests, and each test would have a runtime of like n^2 with a simple approach, so it would increase build time a fair bit. And it's not clear how valuable this sort of test is |
|
Thanks to @bekos research at #1098 (comment), it became possible understand WHY the repeated date behaviour: date/time = Oct 17, 00:00 ... "plus 1" becomes Oct 18, 00:00 I've just tried at the browser's JavaScript console - and it happens like above. Due to the DST (Daylight Saving Time), somehow informed in the localization, the JavaScript "setDate" becomes unreliable. Right from my console: You see? A @bekos wondered "probably the whole directive should work with UTC dates"; I didn't know what to say at that time. Now, after some thinking and tests, my opinion is YES, the whole directive should work with UTC functions - this way no localization tricks will affect. Why, in short, 19 + 1 = 19? Answer, in short: due to DST/localization tweaks entering the scene. The clock is altered, localization is aware, result is more or less unpredictable (here in Brazil DST rules change according to the politicians mood). So, for the datepicker to be in the safe and secure side, let's use the UTC functions. We learned this:
In short:
I'd merge and deploy this PR to the live site immediately, as now we are at October, and the wrong calendar appears to us at AngularUI Bootstrap front page without the need of changing the month... not an easy request, I know, but let me request it anyway. At least, the specific datepicker's "getDates" function becomes safe. Then, people who knows datepicker better may apply the UTC change through the whole directive. Finally, regarding testing, I'm afraid I can't help, except by making questions and wondering:
|
|
Another possibility would be to generate the calendar not using these built-in JS functions... |
|
@tersariolli - Indeed. I've checked February 2015, and it shows day 21 twice! The solution does not work... I'd like to know why... Have you seen this approach here? #1196 - Certainly not optimal - but as a workaround it seems to "fix" the issue better than this flawed PR. A proper solution would be another approach, not necessarily using these built-in JavaScript getDate / setDate "unpredictable" functions... I mean: a complete rewrite, not a quick-and-dirty small fix. |
|
@jbruni wouldn't working with the UTC dates be a solution here? Planning to look into it in the near future and the issue boils down to being able to reliably add one day to a given date. |
|
From these dates is used only the date part, time part is irrelevant. So you can initialize the first date at 12pm (noon) and add 1 day without caring about timezone changes. |
|
@pkozlowski-opensource - Surprisingly not, it seems. I've just checked February 2015 in my app (which is running with the patch applied), and it repeats the day 21. I've not investigated further... By now, I'm only surprised and intrigued... Before any further consideration, it seems to me it is easier to "add one day to a given date" using a small set of simple and neat custom build functions - instead of using these built-in ones... |
|
Or as @bekos is saying - by setting mid-day as time these changes won't break the date... This works and this is what really matters... It just does not "look and feel" very nice to have to apply such workarounds, but it is certainly easier, faster and cheaper than coding a whole new set of date calculation functions... |
|
@jbruni interesting... There is always something new we can learn about JavaScript Date APIs :-) Using noon as a work-around should work, but I agree that somehow it "doesn't sound right" :-) |
|
Regarding this, I say to use UTC dates as well. Why:
However, the same cannot be said for a localized time. DST creates "gaps" or "overlapping times" where there's 1 hour of invalid time (during the gap), and 1 hour of time (overlap) that doesn't mean anything unless you say it's with or without DST. The user can then either by take their face value (year, month, day, hour, minute, second) or convert them from UTC to local time (and handle the discrepancies that come along with that). |
ghost
assigned
|
@bekos I'm going to assign this PR for you for the review as you are more familiar with the code-base and I don't know what is the status of the rewrite. |
|
Closing this, as the issue is expected to be solved by #1599 using the "dirty" workaround :-) @chrisirhc I see, but if you are saying to use globally UTC dates for the datepicker, I believe this will create a lot of problems with unexpected behavior. For example you can be at the last day of one month and the calendar to show the title of the next and so on. Other than this issue, using local dates is not a problem until now, and someone can covert to UTC with something like |
No description provided.