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

Fix conversion of dates from C# to V8 #76

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

spahnke
Copy link
Collaborator

@spahnke spahnke commented Mar 19, 2019

It's me again, with another correction of the date conversion πŸ™„

Problems with the current approach

In #75 we introduced a date conversion from C# to V8 using the constructor V8 exposes in their C++ API that only takes one parameter (milliseconds since 1970-01-01 00:00:00), and then using the set* methods on the constructed date object. This is bad however, since there is no single permutation of those methods that always yields the correct value. Sorry for that! πŸ™‡

Example: 1967-04-01 00:00:00
Depending on the timezone the milliseconds from C# could represent 1967-03-31 23:00:00. If we now set the month to April we get the 31st of April which JavaScript interprets as the 1st of May, leading to an incorrect date of 1967-05-01 00:00:00.

This is partly because we used the milliseconds from the C# DateTime object to construct a date value near the desired one and then manipulating it setting the year first, then month, etc. You could remedy that case by reversing the order of the set* methods, setting the date first, and starting at 1970-01-01 00:00:00 for example instead of the current date. But this would lead to other errors as shown in the next example.

Example: 1972-02-29 00:00:00
If we start from 1970-01-01 00:00:00 which was not a leap year, we need to set the year first.
Otherwise, if we set the month to February and the date to 29 we get the 1st of March.

This approach is therefore not feasible at all, leading to this PR.

The new approach

We instead need to use the date constructor available to JS that takes all parts of a date value to correct this. We get to it by querying the global object. This is then equivalent to something like

new Date(1967, 3, 1, 0, 0, 0, 0)

We also added a brute force test that iterates through all dates from 1900-01-01 to 2100-01-01 to have some quality control. This has two caveats though:

  1. It runs for roughly a minute
  2. It fails for certain timezones (see below)

Tested timezones

The results are from the new test SetAndReadDateTimeLocal_DateRange. All other tests pass!

  • International Date Line (UTC-12) βœ”οΈ
  • PDT (UTC-8) βœ”οΈ
  • MT (UTC-7) βœ”οΈ
  • EDT (UTC-5) βœ”οΈ
  • Azores (UTC-1) ❌
  • UTC βœ”οΈ
  • Berlin (UTC+1) βœ”οΈ
  • Paris (UTC+1) βœ”οΈ
  • Warsaw (UTC+1) ❌
  • Moscow (UTC+3) ❌
  • Tokyo (UTC+9) ❌
  • Sydney (UTC+10) βœ”οΈ
  • Samoa (UTC+13) ❌

Oberservations from the failed tests

The reason for these failures is that it is not possible with V8 to contruct these dates at 00:00:00 local time as if that time doesn't exist at all. You can test that by setting the timezone in your operating system and then running the following code in the Chrome Dev Tools

{   
    const date = new Date(1948, 4, 2, 0, 0, 0, 0);
//     date.setHours(0);
//     date.setMinutes(0);
//     date.setSeconds(0);
//     date.setMilliseconds(0);
    console.log(date);
    console.log("year", date.getFullYear());
    console.log("month", date.getMonth());
    console.log("date", date.getDate());
    console.log("hour", date.getHours());
    console.log("minute", date.getMinutes());
    console.log("second", date.getSeconds());
    console.log("millisecond", date.getMilliseconds());
}

We did not manage to get V8 to set the time back to 00:00:00. We also tested the same dates with NodaTime, but as soon as we try to convert back to a local timezone, NodaTime agrees with V8 by setting the time to 01:00:00 (or similar). Both use the IANA timezone database, so we think they are correct. It's as if 00:00:00 did not exist. We currently have no solution to this problem.

Detailed list of failures:

// Azores
Expected 27.03.1977 00:00:00, but got 27.03.1977 01:00:00
Expected 02.04.1978 00:00:00, but got 02.04.1978 01:00:00
Expected 01.04.1979 00:00:00, but got 01.04.1979 01:00:00
Expected 30.03.1980 00:00:00, but got 30.03.1980 01:00:00
Expected 27.03.1994 00:00:00, but got 27.03.1994 01:00:00
Expected 26.03.1995 00:00:00, but got 26.03.1995 01:00:00
// then every year when entering daylight savings time

// Warsaw
Expected 29.04.1945 00:00:00, but got 29.04.1945 01:00:00
Expected 14.04.1946 00:00:00, but got 14.04.1946 01:00:00

// Moscow
Expected 03.07.1916 00:00:00, but got 03.07.1916 00:01:02 ❗️ 
Expected 21.06.1930 00:00:00, but got 21.06.1930 01:00:00
Expected 01.04.1981 00:00:00, but got 01.04.1981 01:00:00
Expected 01.04.1982 00:00:00, but got 01.04.1982 01:00:00
Expected 01.04.1983 00:00:00, but got 01.04.1983 01:00:00
Expected 01.04.1984 00:00:00, but got 01.04.1984 01:00:00

// Tokyo
Expected 02.05.1948 00:00:00, but got 02.05.1948 01:00:00
Expected 03.04.1949 00:00:00, but got 03.04.1949 01:00:00
Expected 07.05.1950 00:00:00, but got 07.05.1950 01:00:00
Expected 06.05.1951 00:00:00, but got 06.05.1951 01:00:00

// Samoa
Expected 01.01.1950 00:00:00, but got 01.01.1950 00:30:00
Expected 26.09.2010 00:00:00, but got 26.09.2010 01:00:00
Expected 30.12.2011 00:00:00, but got 31.12.2011 00:00:00

Conclusion

We sincerely think the approach of this PR is the best/most correct we can get, since both worlds unfortunately inherently disagree. Looking forward to your opinions!

Thanks!

/cc @Ablu

We need to use the date constructor to create a date, otherwise the
order of invocation of the set methods influences the result leading to
incorrect date values. There is no permutation which is always correct.

Example: 1967-04-01 00:00:00
Depending on the timezone the milliseconds from C# could represent
1967-03-31 23:00:00. If we now set the month to April we get the
31st of April which JavaScript interprets as the 1st of May.

Even if we don't use the current date as starting point but
1970-01-01 00:00:00 using the set methods can lead to errors.

Example: 1972-02-29 00:00:00
Since we start from 1970-01-01 00:00:00 (UTC) which was not a leap year,
we need to set the year first. Otherwise, if we now set the month to
February and the date to 29 we get the 1st of March.

Another case is can occur when changing daylight savings time.
Copy link
Contributor

@oliverbock oliverbock left a comment

Choose a reason for hiding this comment

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

Calendars are hard to get right! I don't pretend to understand the subtleties of the Samoan timezone, but I think the new approach is better: everyone has the same ideas about the Gregorian calendar and clocks.

@spahnke
Copy link
Collaborator Author

spahnke commented Mar 20, 2019

@oliverbock Friendly ping to merge πŸ˜„

@cesaryuan
Copy link

Hi! My timezone is UTC+8. So it means the bug cannot be fixed?

@oliverbock
Copy link
Contributor

@cesaryuan I think the comments here are mainly regarding a problem with testing, not realistic use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants