Skip to content

feat: Add RelativeTimeFormat support#1708

Closed
ws-rush wants to merge 6 commits intolingui:mainfrom
ws-rush:main
Closed

feat: Add RelativeTimeFormat support#1708
ws-rush wants to merge 6 commits intolingui:mainfrom
ws-rush:main

Conversation

@ws-rush
Copy link
Copy Markdown

@ws-rush ws-rush commented Jun 22, 2023

No description provided.

@vercel
Copy link
Copy Markdown

vercel bot commented Jun 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 2, 2023 10:42am

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 22, 2023

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.76 KB (0%)
./packages/detect-locale/dist/index.mjs 721 B (0%)
./packages/react/dist/index.mjs 1.59 KB (0%)
./packages/remote-loader/dist/index.mjs 7.24 KB (0%)

@timofei-iatsenko
Copy link
Copy Markdown
Collaborator

  1. How it supposed to be used?
  2. There are no tests for this functionality
  3. There strange import "util/types"

@ws-rush ws-rush changed the title Add Intl.Relativedate support feat: Add Intl.Relativedate support Jun 22, 2023
@ws-rush ws-rush changed the title feat: Add Intl.Relativedate support feat: Add RelativeTimeFormat support Jun 22, 2023
@ws-rush
Copy link
Copy Markdown
Author

ws-rush commented Jun 22, 2023

  1. How it supposed to be used?
  2. There are no tests for this functionality
  3. There strange import "util/types"

I added test now, I want figure out how to compile code with this to build example, I wish if there are instructions -if it is ok- then, I will add doc later.

code should use as: i18n.relative(0, 'day' {numeric: 'auto'}), this will return today, it uses Intl.RlativeTimeFormat.

@timofei-iatsenko
Copy link
Copy Markdown
Collaborator

I added test now, I want figure out how to compile code with this to build example, I wish if there are instructions -if it is ok- then, I will add doc later.

There is contribution guide, it has instructions how to compile and use in your host project or example.

i18n.relative(0, 'day' {numeric: 'auto'}),

I don't see where this method is bound to i18n instance.

@ws-rush
Copy link
Copy Markdown
Author

ws-rush commented Jun 24, 2023

i18n.relative(0, 'day' {numeric: 'auto'}),

I don't see where this method is bound to i18n instance.

thank you I will review how to bind to i18n instance

unit: Intl.RelativeTimeFormatUnit,
format?: Intl.RelativeTimeFormatOptions
): string {
if ((isDate(value) || isString(value)) && unit) throw new Error('with units you should use numbers as values')
Copy link
Copy Markdown
Collaborator

@vonovak vonovak Jun 24, 2023

Choose a reason for hiding this comment

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

I'm not a maintainer, so take this only as a recommendation, but I think the error message can be improved a little. eg. it could contain the value.

I also wonder if it's needed to catch this case; what error is thrown by Intl.RelativeTimeFormat in this case?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed error message, I planned to make relative accept date or string and generate (value, unit) from it, I will ignore that for now.

@andrii-bodnar andrii-bodnar marked this pull request as draft June 29, 2023 07:33
@ws-rush
Copy link
Copy Markdown
Author

ws-rush commented Jul 2, 2023

I bind it to i18n instance now.

@vonovak
Copy link
Copy Markdown
Collaborator

vonovak commented Jul 12, 2023

I'm going to raise a few bigger questions here:

Do we need relative date formatting in the library?
Do we need date formatting in the library?
Do we need number formatting in the library?

Where I'm going is this: we do need some things - for example, we probably use number formatting when working with plurals (I didn't check, I'm just guessing here).

Do we need date formatting? It seems like we don't? Maybe it's just a convenience functionality that was added at some point?

But I'd like to warn against adding convenience functions "just because". In my opinion, Lingui should not expose too many functions that are only wrappers over the Intl api if the library itself has no use for them. There are many Intl apis and, by extension, if we were to wrap them all we'd just end up with a bunch of bloat. I think in this case, we should not add this functionality.

Thank you :)

@ws-rush
Copy link
Copy Markdown
Author

ws-rush commented Jul 18, 2023

@vonovak, I prefer to manage my internationalization code from one place, I need make 20 day ago in my code, should I use Intl directly and configure languages with this piece or it is better to use it through linguijs and configure it once?

About date and number, I think formatting provided before by linguijs with their code, the difference they now move to use browser API to manage them, so does we need them? yes, we need them, and I think other libraries provide them.

if you think there is better solution, I will be thankful for you to share.

@timofei-iatsenko
Copy link
Copy Markdown
Collaborator

BTW i also think that this could be solved in the userland with a little effort. And it's better not bloat a library with optional extras. Current core architecture made in the way that all this additional methods are not tree-shakeable. So every addition will be added to every bundle where lingui is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants