-
-
Notifications
You must be signed in to change notification settings - Fork 294
feat: Use numeric: 'auto'
for relative times that don't need to be rounded for a given unit
#1872
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,15 +62,6 @@ function resolveRelativeTimeUnit(seconds: number) { | |
return 'year'; | ||
} | ||
|
||
function calculateRelativeTimeValue( | ||
seconds: number, | ||
unit: Intl.RelativeTimeFormatUnit | ||
) { | ||
// We have to round the resulting values, as `Intl.RelativeTimeFormat` | ||
// will include fractions like '2.1 hours ago'. | ||
return Math.round(seconds / UNIT_SECONDS[unit]); | ||
} | ||
|
||
type Props = { | ||
locale: Locale; | ||
timeZone?: TimeZone; | ||
|
@@ -309,24 +300,40 @@ export default function createFormatter(props: Props) { | |
} | ||
|
||
const dateDate = new Date(date); | ||
const seconds = (dateDate.getTime() - nowDate.getTime()) / 1000; | ||
|
||
// Rounding is fine here because `Intl.RelativeTimeFormat` | ||
// doesn't support units smaller than seconds. | ||
const seconds = Math.round( | ||
(dateDate.getTime() - nowDate.getTime()) / 1000 | ||
); | ||
|
||
if (!unit) { | ||
unit = resolveRelativeTimeUnit(seconds); | ||
} | ||
|
||
// `numeric: 'auto'` can theoretically produce output like "yesterday", | ||
// but it only works with integers. E.g. -1 day will produce "yesterday", | ||
// but -1.1 days will produce "-1.1 days". Rounding before formatting is | ||
// not desired, as the given dates might cross a threshold were the | ||
// output isn't correct anymore. Example: 2024-01-08T23:00:00.000Z and | ||
// 2024-01-08T01:00:00.000Z would produce "yesterday", which is not the | ||
// case. By using `always` we can ensure correct output. The only exception | ||
// is the formatting of times <1 second as "now". | ||
opts.numeric = unit === 'second' ? 'auto' : 'always'; | ||
// We have to round the resulting values, as `Intl.RelativeTimeFormat` | ||
// would include fractions like '2.1 hours ago'. | ||
const unitValue = seconds / UNIT_SECONDS[unit]; | ||
const rounded = Math.round(unitValue); | ||
|
||
// `numeric: 'auto'` works well for formatting values that don't | ||
// have a fractional part (e.g. "yesterday") | ||
// | ||
// However, it should not be used with rounded values, as the given | ||
// dates might cross a threshold were the output isn't correct anymore. | ||
// Example: 2024-01-08T23:00:00.000Z and 2024-01-08T01:00:00.000Z would | ||
// produce "yesterday", which is not the case. By using `always` in this | ||
// case, we can ensure correct output. | ||
// | ||
// Note that due to approximations being used for months and years, it's | ||
// practically impossible to trigger the cases "last month" or "last year". | ||
if (unitValue === rounded) { | ||
opts.numeric = 'auto'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this always override to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of having this as the default, but we should give the flexibility to allow the user to opt out if necessary. I think that might be better, rather than trying to expend effort to figure out a justification of why we shouldn't do it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also unsure what the best way really is here. Some aspects to consider: Differences to
Instead, it's a function to produce readable relative time output and internally does:
So I think some deviations from Using It requires date calculations on the consumer side, also taking into account the current time zone. If the user would be able to simply pass It could break in other cases though, due to the rounding that Generally, it's quite a lot of effort to support cases like "yesterday" (see the code snippet in the docs). Which special cases does CLDR provide? CLDR seems to mostly has special cases for
Maybe it would help to create a script that extracts all currently known cases to get a better idea what My main question is: Are there cases where
Due to an approximation that If this is the output you need, you might be better off using Format.js allows passing FormattedRelativeTime allows to set It's interesting to see, but I really wonder how many times users run into issues like incorrectly rounded values. They don't have any validation against rounded values, so they make it really easy to trigger cases like "yesterday", but this will break in many cases. I'm not sure if this is a reasonable solution that I'd like to provide. So there are still some open questions here for me and I'm not really confident what the best solution is. Another option would be to provide a separate function that mimics Example: format.relativeTimeValue(1, 'day', {numeric: 'auto'}); This would put more work into the hand of a user, but gives you all the flexibility. It's almost the same as using So these are my current thoughts. I created this PR to explore the topic in more depth, but I didn't really come to a place yet where my questions are cleared up unfortunately … What's your position? |
||
} | ||
|
||
const value = calculateRelativeTimeValue(seconds, unit); | ||
return formatters.getRelativeTimeFormat(locale, opts).format(value, unit); | ||
return formatters | ||
.getRelativeTimeFormat(locale, opts) | ||
.format(rounded, unit); | ||
} catch (error) { | ||
onError( | ||
new IntlError(IntlErrorCode.FORMATTING_ERROR, (error as Error).message) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Been scratching my head a bit over this code snippet. I think it should now be correct, but it's a bit verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically what I had to do anyways with my current approach (with the exception of using Luxon instead of date-fns) to ensure that
numeric: auto
gave the proper localized text for "yesterday", "today", etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's a way to simplify this, unless we allow passing in an optional config param like
shouldNormalizeDate: boolean
to theformat.relativeTime()
options - but that gets into the issue of requiringnext-intl
to supply its own "start of day" function calculation if it wants to be date-library agnostic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is also what to normalize to. The start of the day is an example here, but you might normalize to a week (e.g. "last week"). I don't think we can make an assumption here.