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

Intl format relative #2173

Merged
merged 26 commits into from
Jun 13, 2022
Merged

Intl format relative #2173

merged 26 commits into from
Jun 13, 2022

Conversation

tan75
Copy link
Contributor

@tan75 tan75 commented Jan 24, 2021

New Feature

Implement intlFormatDistance for date-fns

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Great job, code and tests look good.

Besides review comments I have few requests:

  • Please rewrite it to TypeScript. If you never worked with it or just need advice - ping me, I'll help you.

  • Let's change the behavior. You can see that our formatDistance picks the most appropriate unit depending on the distance. The less the distance the smaller the unit. Let emulate this algo but allow users pass unit as well. So that we can move unit to options and make it optional.

    Here's the algo I suggest (tell me if you have other ideas)

    • Less than minites: second
    • Less than hour: minute
    • Less than day: hour
    • Less than week: day
    • Less than quarter: month
    • Less than year: quarter
    • More or equal to year: year
  • Let's rename the function to intlFormatDistance and make API similar to https://date-fns.org/v2.16.1/docs/formatDistance. Initially, I wanted to keep it similar to Intl API, but realized that it will confuse our users.

  • Add a second date argument like in formatDistance and use it in calculations instead of new Date().

  • Add another function variation intlFormatDistanceToNow similar to https://date-fns.org/v2.16.1/docs/formatDistanceToNow that uses Date.now() as the second argument.

src/intlFormatRelative/index.js Outdated Show resolved Hide resolved
src/intlFormatRelative/index.js Outdated Show resolved Hide resolved
src/intlFormatRelative/index.js Outdated Show resolved Hide resolved
src/intlFormatRelative/index.js Outdated Show resolved Hide resolved
src/intlFormatRelative/index.js Outdated Show resolved Hide resolved
src/intlFormatRelative/index.js Outdated Show resolved Hide resolved
src/intlFormatRelative/test.js Outdated Show resolved Hide resolved
src/intlFormatRelative/test.js Outdated Show resolved Hide resolved
@tan75
Copy link
Contributor Author

tan75 commented Jan 28, 2021

WIP:

  • intlFormatDistanceToNow
  • Test Cases

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Good progress. Please format the code using Prettier and address my suggestions.

src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
Comment on lines 47 to 48
* For reference see:
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/RelativeTimeFormat/RelativeTimeFormat
Copy link
Member

Choose a reason for hiding this comment

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

Instead of forcing users to read MDN, we can add either example for each option variant or a table to the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this ok ?
e.g. "hi": Hindi (language) or "de-AT": German (language) as used in Austria (region)

src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/test.ts Outdated Show resolved Hide resolved
src/constants/index.ts Outdated Show resolved Hide resolved
@robertvanhoesel
Copy link

robertvanhoesel commented Feb 24, 2021

Awesome work! While working on my own implementation I found a gotcha with numeric: 'auto' which might be wise to address (or at least mention in documentation). Because RelativeTimeFormat only takes a value and unit, it has no context/awareness of the actual difference in years.

When I compare two dates, today (Feb 24, 2020) and almost 2 years ago (March 30, 2019).dateFns.differenceInYears will return '-1', which is correct, it is not 2 years ago, just one and a bit.

new Intl.RelativeTimeFormat('en', { 
  numeric: "auto" 
}).format(-1, 'year')
// 'last year'

The majority of Intl implementations will return last year in response to above code, which to humans is just incorrect when we are aware of comparing 2021 and 2019, which was not 'last year'.

https://codesandbox.io/s/musing-river-90m6t?file=/src/index.ts

Since your proposed implementation always takes two dates, you could prevent this error from occurring by having additional checks in the unit resolver that only applies 'auto' when the two compared years differ exactly 1 year, and force 'numeric' in all other cases.

@kossnocorp It occurs to me that maybe this more opinionated/smart logic should be in Intl.RelativeTimeFormat instead? Alongside rounding instead of flooring (i.e. 1 year 9 months becomes 2 years) and optional joining in localized time?

@robertvanhoesel
Copy link

Or... split this implementation to:

  • intlFormatDistanceStrict which uses numeric: always and takes 'ceil', 'round', 'floor' arguments.
  • intlFormatDistance which uses numeric: auto where applicable and generally rounds the years.

My 2cts would be to avoid providing the exact arguments of Intl.RelativeTimeFormat and allow room to algorithmically use the right unit and formatting options.

An other thing to review would be wether outputting 'quarters' by default makes sense to most implementations. Date-fns is slightly opinionated in its other implementations (i.e. when to include time in 'relative' and never returning '4 quarters ago' in formatDistance), providing that as an option to control would make this more useful, otherwise I would find myself going back to first comparing the dates, then calling intlFormatDistance with unit months when I know we're talking about 3-12 months distance.

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Some more new comments

src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
@kossnocorp
Copy link
Member

@robertvanhoesel, this is an excellent point. Adding special logic for numeric: 'auto' and -1/1/0 makes sense at first glance, not only for year but also month and quarter. However, I'm afraid it's a slippery slope. For instance, this is not a problem for minute and hour in English, but I imagine it might be a case for other languages or Intl implementations or wise versa might not have an issue with year.

Using ceil/floor won't help because rounding 4 months to 0 years while working with December will produce this year and its inaccuracy.

I think instead we can use differenceInCalendarDays, differenceInCalendarWeeks, differenceInCalendarMonths and differenceInCalendarYears unless numeric is set to always. Then with your example, you'll get -2 "2 years ago".

We also should make numeric: 'auto' the default value because such:

new Intl.RelativeTimeFormat('en').format(0, 'year')
//=> "in 0 years"

..it is not a good default option. However, I'm not sure what are side effects of this change. @tan75 could you please explore that?

@robertvanhoesel @tan75 @leshakoss @rikkalo WDYT?

@tan75
Copy link
Contributor Author

tan75 commented Mar 17, 2021

@kossnocorp
If numeric: auto is a default option, then when adding style: short it falls back to numeric: always (if the result is 1 day or 1 month or 1 year) e.g.:

const result = intlFormatDistance(
      new Date(1985, 4, 5, 10, 30, 0),
      new Date(1985, 4, 4, 10, 30, 0),
      { style: 'short' }
) //=> 'in 1 day' instead of 'tomorrow'

however it looks like explicitly adding numeric: auto when calling the function will fix it (even though we know that's the case by default):

    const result = intlFormatDistance(
      new Date(1985, 4, 5, 10, 30, 0),
      new Date(1985, 4, 4, 10, 30, 0),
      { style: 'short', numeric: 'auto' }
   )//=> 'tomorrow'

The same happens to 1 month and 1 year
Test

@tan75
Copy link
Contributor Author

tan75 commented Apr 17, 2021

hi @robertvanhoesel
my tests are failing on this Error

image

Any ideas why?

@robertvanhoesel
Copy link

@tan75 the typescript compiler needs to know that you are using es2020.intl definitions. In .tsconfig you should add es2020 to the target. Also, this is a good case to catch, as older browser will also not know about this constructor and fail.

@robertvanhoesel
Copy link

robertvanhoesel commented Apr 17, 2021

I use the below logic to check wether the API is available in the browser.

const IntlRelativeSupported = typeof Intl.RelativeTimeFormat === 'function';

@tan75
Copy link
Contributor Author

tan75 commented Apr 18, 2021

@tan75 the typescript compiler needs to know that you are using es2020.intl definitions. In .tsconfig you should add es2020 to the target. Also, this is a good case to catch, as older browser will also not know about this constructor and fail.

Thank a lot for your answer! Do you mean tsconfig.json? I added it here
tsconfig
But is it still failing:
Run

Copy link
Contributor

@leshakoss leshakoss left a comment

Choose a reason for hiding this comment

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

⭐️

src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/constants/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/test.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@tan75 tan75 mentioned this pull request Dec 5, 2021
24 tasks
@tan75 tan75 changed the title Intl format relative2 Intl format relative Dec 28, 2021
@tan75 tan75 requested a review from leshakoss December 29, 2021 21:39
@tan75 tan75 mentioned this pull request May 18, 2022
5 tasks
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
src/intlFormatDistance/index.ts Outdated Show resolved Hide resolved
tan75 and others added 13 commits June 11, 2022 20:16
Co-authored-by: Sasha Koss <koss@nocorp.me>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Co-authored-by: Sasha Koss <koss@nocorp.me>
Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

👏

@kossnocorp kossnocorp merged commit 17fe334 into master Jun 13, 2022
@kossnocorp kossnocorp deleted the intlFormatRelative2 branch June 13, 2022 05:53
@orhangazi
Copy link

orhangazi commented Sep 29, 2022

Hi, I can't pass this error. I am using react native version is 0.68.2, react version is 17.0.2 and using date-fns version is 2.29.3. Code reference doc here. I am using also intl version is 1.2.5. Maybe this is to cause the error?

I check with this code

const IntlRelativeSupported = typeof Intl.RelativeTimeFormat === 'function'
console.log("IntlRelativeSupported: ", IntlRelativeSupported) --> output: false. what should I do?

Codes:

import { format, formatDistance, formatRelative, subDays, intlFormatDistance } from 'date-fns'
.....
lastMessageDeliveryDate = intlFormatDistance(
			new Date(1986, 3, 4, 11, 30, 0),
			new Date(1986, 3, 4, 10, 30, 0)
		)

I am getting this error:
image

* @throws {RangeError} `options.style` must not be invalid style
*
* @example
* // What is the distance between the dates when the fist date is after the second?
Copy link

Choose a reason for hiding this comment

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

Typo - first

@JestemTrzcinska
Copy link

Hi, I also encountered this problem. The same result as here.
It works good on newer iOS simulators, but it doesn't on Android simulators and old iPhone simulators like iPhone 6s with iOS 13.7.

Any workaround? I need something exactly like intlFormatDistance with its "... ago" etc.

Making if statement is not good way for me, because I need it to work on every devices.

"date-fns": "^2.29.3",
"react": "18.2.0",
"react-native": "0.71.6",

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

Successfully merging this pull request may close these issues.

intlFormatDistance API development
7 participants