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

Editorial: Introduce Record for relativeTo #2837

Closed
Tracked by #2950
ptomato opened this issue May 10, 2024 · 13 comments
Closed
Tracked by #2950

Editorial: Introduce Record for relativeTo #2837

ptomato opened this issue May 10, 2024 · 13 comments
Assignees
Milestone

Comments

@ptomato
Copy link
Collaborator

ptomato commented May 10, 2024

After #2826 lands, consider introducing a relativeTo Record in the spec, to show implementations how relativeTo can be handled in a more statically-typed way.

There might be an opportunity to give a formal shape to "relativeTo". Kevin Ness brought this up in the last meeting regarding difficulty in statically typed languages.

type RelativeTo = {
  plainDateTime: Temporal.PlainDateTime
  timeZone: Temporal.TimeZone // or null if UTC?
}

function normalizeRelativeTo(input): RelativeTo {
  if (input instanceof Temporal.ZonedDateTime) {
    return {
      plainDateTime: input.toPlainDateTime(),
      timeZone: input.getTimeZone(),
    }
  } else if (input instanceof Temporal.PlainDate) {
    return {
      plainDateTime: input.toPlainDateTime(), // gives 00:00:00
      timeZone: new Temporal.TimeZone('UTC')
    }
  }
}

/*
Adds a Duration to the relativeTo and returns an epoch nanoseconds
This function is only ever called with time units zeroed out!
(It's just the way our nudging/bubbling system works during relative-rounding)
Thus, we don't need to worry about the intricacies of *when* time units are
added in PlainDateTime vs ZonedDateTime.

A utility function is nice because we don't need to call GetUTCEpochNanoseconds and
GetInstantFor conditionally each time.
*/
function addToRelativeTo(relativeTo: RelativeTo, duration): bigint {
  return relativeTo.timeZone.getInstantFor(
    relativeTo.plainDateTime.add(duration)
  ).epochNanoseconds
}

Originally posted by @arshaw in #2758 (review)

@ptomato
Copy link
Collaborator Author

ptomato commented May 10, 2024

Also use this to eliminate UnbalanceDateDurationRelative as suggested in that review.

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 13, 2024

@arshaw I might need some input from you on this. What do you do in the fullcalendar polyfill if the ZonedDateTime passed as relativeTo has an ambiguous wall-clock time? You can convert it to the PlainDateTime in the RelativeTo struct, but then you couldn't convert it back.

@arshaw
Copy link
Contributor

arshaw commented Sep 13, 2024

Hi @ptomato, I devised this during the Duration rounding refactor, but I haven't actually implemented this RelativeTo record stuff in the fullcalendar polyfill yet. I can do that and report back. Will be sometime in Oct probably.

I'm not quite sure what you mean though. I'm sort-of converting it back (to an epochNanoseconds) in the addToRelativeTo function above.

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 13, 2024

@arshaw But if the original ZonedDateTime was, say, 2023-11-05T01:30-05:00[America/New_York], it becomes 2023-11-05T01:30 in the RelativeTo record, then when you go to convert it back to epochNanoseconds in addRelativeTo ... there are two possible epochNanoseconds.

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 13, 2024

Oh! Does it not matter because you never need the original ZonedDateTime back? You only add or subtract date duration units, and so if you land on an ambiguous wall clock time you actually want the disambiguation: 'compatible' behaviour...

@arshaw
Copy link
Contributor

arshaw commented Sep 13, 2024

@ptomato yes exactly

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 14, 2024

I've written up a draft of this refactor in https://github.com/tc39/proposal-temporal/compare/main...2837-relativeto-record?expand=1

It turns out we do need the original ZonedDateTime in only one place: the check at the start of DifferenceZonedDateTimeWithRounding where we return difference-of-instants if largestUnit is a time unit. I don't have a solution for that yet.

It's also slightly annoying, but doable, that you have to compute the sign separately and pass it in to DifferenceZonedDateTimeWithRounding.

Other than those, I think this works pretty well.

@arshaw
Copy link
Contributor

arshaw commented Sep 19, 2024

Hey @ptomato, that's a great start, though if we can't figure out those two issues, it probably creates more problems than it solves. I want to take a more in-depth look at all this, and potentially devise a way to avoid passing a relativeTo into DifferenceZonedDateTimeWithRounding altogether (and possibly refactor-away DifferenceZonedDateTimeWithRounding somehow, as it's no longer really diffing two zoneddatetimes anymore) but I won't have time before the meeting. Hopefully shortly after.

@ptomato ptomato added this to the Stage "3.5" milestone Sep 20, 2024
@ptomato
Copy link
Collaborator Author

ptomato commented Sep 20, 2024

If we decide to do this, we should do it before the TC39 meeting October 8. Ideally we wouldn't make any big refactors after that.

@arshaw
Copy link
Contributor

arshaw commented Oct 2, 2024

Hi @ptomato, I looked this over and really couldn't find a way to solve those issues. DifferenceZonedDateTimeWithRounding really should be receiving two epochNanoseconds, and the fact that the RelativeTo record does not afford this makes me think RelativeTo should simply be [plainDate | null, zonedDateTime | null], which is essentially what we already have. Sorry about the wild goose chaise.

@arshaw
Copy link
Contributor

arshaw commented Oct 2, 2024

Is there still a way to remove UnbalanceDateDurationRelative though? That old "balancing" concept isn't really used anywhere else I don't think.

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 2, 2024

@arshaw Thanks for investigating! Much appreciated.

As for UnbalanceDateDurationRelative, I think at a minimum we should rename it if we do #2953. We could probably do a smaller version of this refactor only in Duration.compare and get rid of UnbalanceDateDurationRelative that way. I'll check if that's feasible.

@ptomato ptomato closed this as completed Oct 2, 2024
@ptomato
Copy link
Collaborator Author

ptomato commented Oct 3, 2024

I tried refactoring only Duration.compare. By itself, it isn't really an improvement over the simplicity/readability of the status quo. So I think we should just go with renaming UnbalanceDateDurationRelative in #2953.

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

No branches or pull requests

2 participants