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

Why so many classes? #927

Closed
stebogit opened this issue Sep 21, 2020 · 3 comments
Closed

Why so many classes? #927

stebogit opened this issue Sep 21, 2020 · 3 comments
Labels

Comments

@stebogit
Copy link

stebogit commented Sep 21, 2020

I want to expand the concept I mentioned here. Please take this as an attempt to contribute with a different view, not to criticize your work.

The basic idea is that an "instant in time" on earth is always uniquely identified by a date, a time and a timezone.
However, the timezone is by definition an offset (although complex) from the absolute/unix/epoch timezone, which is UTC.
This essentially means that what really defines the instant is its reference to the UTC date and time, while the timezone can be modified ad libitum to localize the time of the event. In addition, a timestamp without a timezone is equivalent to a UTC timestamp, that is UTC is always, as a matter of fact, the default timezone of a timestamp when not otherwise specified.

With this idea in mind I wonder about the need of the following classes: Absolute, DateTime, Time, Date, YearMonth and MonthDay. Specifically I'd say that just the single LocalDateTime (or however it will be called) should be the only class used here to represent the underlying concept of "instant" and allow to handle all cases. This would drastically reduce the overall complexity and possible confusion over the Temporal object and its API, including its implementation and code maintenance.

The fundamental condition would be having, for each LocalDateTime instance, an immutable reference to the UTC timestamp (the timezone could be altered) and default time to 00:00:00.00000, date to the current date and timezone to UTC, if these were not passed to the instance constructor. This might already be the case, but I did not look that deep into its implementation at this point.

In particular, each of the "redundant" classes can be thought of a special case of a LocalDateTime:

  • Absolute: a UTC referenced LocalDateTime instance
  • DateTime: same as above
  • Time: any UTC LocalDateTime (to ignore DST) where only the time is of relevance
  • Date: any LocalDateTime instance where only the date is referenced/changed
  • YearMonth: same as above, but both year and month are referenced/changed
  • MonthDay: same as above, but both month and day are referenced/changed

For example, why do you need a dedicated class to handle the concept of "wall-clock" or a "date with wall-clock" if you can do all the same operations with a UTC instance of the LocalDateTime?
In other words, considering these instances

const utc = '2020-01-03T19:41:00Z';
const clock1 = Temporal.Time.from('19:41:00');
const clock2 = Temporal.LocalDateTime.from(utc);

to represent a clock and its behavior, is there any difference between them in how they represent the clock? That is, aren't all methods available for clock1 also (necessarily) available for clock2?
Or if I instantiated these two variables to represent the datetime of an event

const absolute = Temporal.DateTime.from('2020-01-03T19:41:00');
const datetime = Temporal.LocalDateTime.from('2020-01-03T19:41:00Z');

since they both represent a "date+time", aren't all the methods available for absolute (necessarily) available for datetime as well; and, in addition, don't those methods return the same result for both instances?
Thus why defining different classes for things that can be equally represented with the same more generic/versatile class?

I do believe this approach could bring significant benefit to the project. However I would be glad to learn more about your thought process and maybe details I don't know which do justify the status quo.

@justingrant
Copy link
Collaborator

Hi @stebogit - thanks for the feedback and suggestions!

You're correct that LocalDateTime is the most comprehensive type in Temporal. This is why it can be converted into any other Temporal type (except Duration) without needing any parameters on the conversion methods. You're correct that it would be possible to have a Temporal API that's only (or primarily) that one type.

That said, we think this would be a bad idea, because it's very common for applications to deal with data that is not comprehensive. For example, a date picker component only knows about dates not times. A time picker component knows about times but not dates. A birthday in a user's profile doesn't know about years only the month and day. A log file doesn't know the time zone of the server where the log is being recorded, only seconds-since-epoch. And so on.

By offering strongly-typed classes to handle popular "subset" use cases, there's many benefits:

  • Trying to access data outside of the domain of that subset will cause an exception which makes it easy to identify out-of-scope data access.
  • Users writing in modern IDEs can use autocomplete, eslint, etc. to identify out-of-scope data access even without running any code.
  • Subset-specific use cases can be made more ergonomic without adding risk to other subsets' use cases. For example, converting from a Temporal.Date to a Temporal.LocalDateTime can make the time parameter optional because midnight is a reasonable default which supports a very popular use case: "get a local-timezone date range in UTC". However, the conversion function from Temporal.Time to Temporal.LocalDateTime will make the date parameter required, because there is no "default date" that makes sense for most use cases.
  • Subset-specific types reduce the opportunity for unexpected bugs in the same way that software security relies on reducing the attack surface. By reducing the number of things that a type can do, it reduces the opportunity for things to go wrong. For example, if I pass a Temporal.MonthDay to a library method that expects a Temporal.Date, it's easy for the library method to tell me that I used the wrong type. This would not be possible if all types were the same.
  • The current JS Date type supports these use cases exactly as you've indicated: as a single type. Much of the design of Temporal stems from experience with problems experienced over 20+ years of dealing with the limitations of legacy Date.
  • By splitting up Absolute, DateTime, and LocalDateTime types, Temporal can ensure that code can always know if its source data knows about its time zone or not. This prevents bugs, especially DST bugs. For example, legacy Date uses an implicit system time zone for all operations. This has been the source of countless bugs in server apps (especially on-premise servers whose local timezone isn't set to UTC) where the app makes assumptions about, for example, the current date which isn't a match for the date in the user's time zone. But many operations don't care about the time zone. By making some subset types local-timezone-ignorant, it makes it possible to work with, for example, UTC timestamps without risking the case where the user adds 48 hours to a UTC timestamp while assuming that the resulting local time will be exactly two calendar days later, which will not be true across a DST transition.

In summary, by splitting into separate "subset" types, Temporal makes it easier to prevent bugs and easier to encourage best-practice patterns for dealing with date and time data.

We know that some developers are currently happy with "the same more generic/versatile class" and would like to continue writing code using that approach. Supporting those developers was one of the reasons for adding LocalDateTime to Temporal. Although many developers will (and, IMHO, should) use the "subset" classes where appropriate, they aren't forced to do so for most use cases. For these developers, the typical Temporal pattern will be to as quickly as possible convert their input into a LocalDateTime and use that type for most operations. Their only interaction with the rest of Temporal will be around parsing and formatting. We hope that over time these developers start using other Temporal types where appropriate, but we won't force them to do so.

For example, why do you need a dedicated class to handle the concept of "wall-clock" or a "date with wall-clock" if you can do all the same operations with a UTC instance of the LocalDateTime?

For "wall-clock" alone, I strongly disagree. There are many use cases that deal with times without any awareness of dates or time zones. For example: clocks, time pickers, etc. This is a popular subset of date/time data.

For "date with wall-clock", I think you have a stronger case. I believe you're correct that the type currently named DateTime will have relatively limited use after LocalDateTime is available, because there are relatively few use cases where you know both the date and the time but don't know the time zone. Furthermore, because the type currently called DateTime doesn't adjust for DST, it can be risky for unaware users if they add or subtract time and expect the results to adjust for DST.

But a "timezone-less date/time" is still a common enough use case that IMHO it deserves its own type-- if only to support interop with other platforms (e.g. SQL Server, .NET, other DBMSs) that make heavy use of such a type, and also to support less-common use cases where the date and time are known but the offset nor time zone is not known.

FYI, we're in the process of renaming both LocalDateTime and DateTime so that the final names will reflect the relatively-more-common use cases for LocalDateTime and the relatively-less-common use of DateTime.

Or if I instantiated these two variables to represent the datetime of an event

const absolute = Temporal.DateTime.from('2020-01-03T19:41:00');
const datetime = Temporal.LocalDateTime.from('2020-01-03T19:41:00Z');

since they both represent a "date+time", aren't all the methods available for absolute (necessarily) available for datetime as well; and, in addition, don't those methods return the same result for both instances?

You're correct that we could consider exposing since-epoch properties on LocalDateTime. I opened #932 to track this suggestion.

That said, Temporal.LocalDateTime.from('2020-01-03T19:41:00Z') will throw an exception, because Z implies a UTC time that does not communicate anything about the local time zone. Without knowing the local time zone, it's impossible for LocalDateTime to do many important things like adjusting for DST when adding, subtracting, or otherwise creating derived values from this. Also, simply knowing the local date or time is not supported without knowing the local time zone.

So this exception is present to prevent developers from accidentally assuming that a UTC time is a local time. FWIW, using an incorrect implicit time zone was one of the top problems with legacy Date that we're trying to prevent in Temporal.

The correct code for that use case is this:

ldt = Temporal.Absolute.from('2020-01-03T19:41:00Z').toLocalDateTime(timeZone, 'iso8601');

The second parameter is the name of the calendar system used. Note that we made some recent decisions to add an ISO-calendar shortcut of that method, so in a few weeks there will be a shorter way to write it:

ldt = Temporal.Absolute.from('2020-01-03T19:41:00Z').toLocalDateTimeISO(timeZone);

@stebogit
Copy link
Author

Thanks @justingrant for the detailed explanation, now I understand the relevance of the different classes.

However I'm still not convinced about the need of two separate classes for Absolute and DateTime. Regardless of their final name, it seems to me they do both represent ultimately the same thing, although currently exposing different methods, which is an absolute/unix/UTC reference instant.

What's the reasoning behind having two separate classes there? Wouldn't one single class (UtcDateTime?) exposing all their methods combined be maybe an option to evaluate?

@ptomato
Copy link
Collaborator

ptomato commented Jan 14, 2021

Sorry this got lost among the other issues. To get back to your question, (although maybe it was answered already in #884?) PlainDateTime doesn't represent an exact instant, because it doesn't have a time zone. So it may represent a nonexistent time or an ambiguous time.

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

No branches or pull requests

3 participants