Skip to content

Overhaul duration calculation functions #294

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

Closed
wants to merge 1 commit into from

Conversation

leduyquang753
Copy link
Contributor

The current implementation of the relative date display appears to be brittle and actually being broken, for instance with this PR's date being 2 Dec 2024, the date 28 Jan 2024 is being very wrongly displayed as "9 days ago".

image

This pull request is an attempt to rebuild the computations from the ground up so as to be more consistent and robust:

  • The relative display format is now implemented using a separate function, taking into account the actual dates in question rather than relying on the information-stripped duration.
  • The duration display format has a new implementation and no longer relies on a reference date.
  • Functions applyDuration and elapsedTime now give consistent results.

Some semantics had to be changed and so I expect questions and probably some alterations to the details.

For "x days ago", a new standalone function is made that takes the original
dates into account. For durations, the rounding to nearest unit function now
works in a more consistent way.
@keithamus
Copy link
Contributor

keithamus commented Dec 2, 2024

Thanks for working on this! This PR seems too large to reasonably review and looking through some of the changes seem to not really change logic but just refactor for code style which makes it harder to review. Is it possible to break this PR down into smaller pieces and optimise for a clean diff?

The relative display format is now implemented using a separate function, taking into account the actual dates in question rather than relying on the information-stripped duration.

Part of the reason for this was to separate out the logic so that it falls inline with Temporal, so that we may one day be rid of this code and instead use the built-in Temporal.Duration's round function.

Copy link
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

I've just left a couple of comments as jumping off points.

Overall I'm excited by this change but I am also a little concerned about how much is changing, especially with the tests. I'm in no way suggesting the current tests are especially correct but I also think we need to assess each one to see if the new value makes more sense, especially which when it comes to calendars (vs time) can be more of a colloquial preference than a hard science. For example we have found users tend to prefer the calendar year being used for such long durations, meaning Jan 2024-Sep 2022 is often rounded up to the much larger "2 years ago" - if we mark it as "1 year ago" we tend to get complaints 😕.

r.setHours(r.getHours() + duration.hours)
r.setMinutes(r.getMinutes() + duration.minutes)
r.setSeconds(r.getSeconds() + duration.seconds)
const actions: Array<() => void> = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see why we need an array here. Because of the if? Having an array of functions and reversing them seems costly compared to duplicating the small number of lines twice in an if/else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the duration to be applied is negative, the order of the operations will be reversed. This is so that adding a duration then subtracting the same duration always gives the original date, which was not the case with the old implementation.

opts?: Partial<RoundingOpts>,
): [number, Intl.RelativeTimeFormatUnit] {
const rounded = roundToSingleUnit(duration, opts)
export function getRoundedRelativeTimeUnit(rounded: Duration): [number, Intl.RelativeTimeFormatUnit] {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is removing an unused variable that might be a great candidate for a smaller PR which can be approved and merged in isolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I have explained initially this is because the new implementation does not rely on a reference date to be passed anymore, but the original implementation did, and so this change can only come along with the new implementation of the function.

@@ -1,6 +1,7 @@
import DurationFormat from './duration-format-ponyfill.js'
import type {DurationFormatOptions} from './duration-format-ponyfill.js'
const durationRe = /^[-+]?P(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)W)?(?:(\d+)D)?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+)S)?)?$/
const durationRoundingThresholds = [Infinity, 11, 28, 21, 55, 55, 900]
Copy link
Contributor

@keithamus keithamus Dec 2, 2024

Choose a reason for hiding this comment

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

I'm a little concerned that this array is a level of indirection which makes it hard to figure out the rounding rules. At minimum it might be worth adding comments to each of these explaining that they're months, days, hours and so-on. (Though I'd prefer to keep them localised to their call-sites for better readability).

Copy link
Contributor Author

@leduyquang753 leduyquang753 Dec 2, 2024

Choose a reason for hiding this comment

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

I just attempted to place the constants at the same place that others appear to be placed, which is to the top of the source file. If putting them next to the function that uses them is better then I will happily do that, in fact I also do prefer it that way. I will be sure to add comments.

Also in case you did not notice, these numbers are in an array because the new rounding routine is generalized, the duration values are put in an array and changed with a loop.

@leduyquang753
Copy link
Contributor Author

This PR seems too large to reasonably review and looking through some of the changes seem to not really change logic but just refactor for code style which makes it harder to review.

This is a complete rework of the logic, you can see the functions for elapsed time and rounding totally reimplemented with little resemblance to the original code, or you can try comparing outputs from the new implementation against the current one. The vast majority of changes are in the test suites, which I have also rebuilt to more comprehensively test the new behavior.

Is it possible to break this PR down into smaller pieces and optimise for a clean diff?

I could split this into multiple parts if that is what you desire, namely the new elapsed time calculation, the new rounding function, the relative time implementation, and the improved duration application function. However adopting them one by one may lead to inconsistent behavior, unless the splitting is just for the purpose of more convenient code review.

@keithamus
Copy link
Contributor

unless the splitting is just for the purpose of more convenient code review.

This is exactly the case. I am happy for us to release as one but it would be nice to get more detail about the changes in a few separate PRs especially when it comes to the tests.

@leduyquang753
Copy link
Contributor Author

For example we have found users tend to prefer the calendar year being used for such long durations, meaning Jan 2024-Sep 2022 is often rounded up to the much larger "2 years ago" - if we mark it as "1 year ago" we tend to get complaints 😕.

To specifically address this, my new implementation does output this duration as "in 2 years"/"2 years ago", the code only looks at the year difference in this case.

> new Intl.RelativeTimeFormat("en", {amounts: "auto"}).format(...relativeTime(new Date("2022-09-25"), "second", new Date("2024-01-15")))
'2 years ago'

I also think we need to assess each one to see if the new value makes more sense

I completely understand this and so to make it clear I am not in any rush to get this new behavior merged right away, it will of course take some time to discuss all the little details.

@leduyquang753
Copy link
Contributor Author

This is exactly the case. I am happy for us to release as one but it would be nice to get more detail about the changes in a few separate PRs especially when it comes to the tests.

Thank you for the confirmation. This will be changed to just a draft then and as I make the new set of PRs with more detailed descriptions it will be closed in favor of them.

@leduyquang753 leduyquang753 marked this pull request as draft December 2, 2024 10:01
@leduyquang753
Copy link
Contributor Author

Closing in favor of split pull requests.

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.

2 participants