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

BigInt support for useFormatter.format() #222

Closed
tacomanator opened this issue Mar 23, 2023 · 4 comments · Fixed by #225
Closed

BigInt support for useFormatter.format() #222

tacomanator opened this issue Mar 23, 2023 · 4 comments · Fixed by #225
Labels
enhancement New feature or request

Comments

@tacomanator
Copy link
Contributor

Is your feature request related to a problem? Please describe.

In TypeScript, cannot easily use BigInt with useFormatter().format(value) as only accepted type of value is number.

Describe the solution you'd like

Change type of value parameter of useFormatter().number() function from number to number | BigInt.

Describe alternatives you've considered

Add a new function as opposed to changing existing, if compatibility is a concern. Note, however, that Intl.NumberFormat(...).format(value) does support BigInt.

@tacomanator tacomanator added enhancement New feature or request unconfirmed Needs triage. labels Mar 23, 2023
@amannn
Copy link
Owner

amannn commented Mar 23, 2023

Hello and thanks for the question!

Change type of value parameter of useFormatter().number() function from number to number | BigInt.

That sounds reasonable to me. I haven't worked with BigInt so far, do you know if there are any drawbacks to consider?

I'm currently quite busy with the RSC integration, so if you'd like to contribute this feature, I'd be happy to review a PR!

@amannn amannn removed the unconfirmed Needs triage. label Mar 23, 2023
tacomanator added a commit to tacomanator/next-intl that referenced this issue Mar 24, 2023
@tacomanator
Copy link
Contributor Author

Would be happy to. I made the change to a forked repo and I'd like to test it in a local project before submitting the PR, but I don't seem to be able to install from local or forked GitHub project as I normally would. Tried install from file:../next-intl/packages/next-intl but subsequent imports fail. Sorry for the basic question—I haven't used learna in this way before.

@amannn
Copy link
Owner

amannn commented Mar 24, 2023

Thank you so much! 🙌

The easiest way is probably to add some tests in createFormatter.test.tsx and useFormatter.test.tsx.

The packages starting with "example" are also linked to the local dependencies, you can use those to test the feature in an e2e way if you like! However, the examples are currently intended to demonstrate specific aspects that need to be pointed out, I think I wouldn't change them to show BigInt support though.

Please note that to run the examples, you need to build use-intl and next-intl locally first. This is a bit hidden knowledge currently, I should probably set up a contributors guide that mentions this.

Let me know how it goes!

tacomanator added a commit to tacomanator/next-intl that referenced this issue Mar 24, 2023
@tacomanator
Copy link
Contributor Author

Thank you much. For now, I added two tests and broke one of the existing tests into two parts. All passing—sending PR over.

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

Successfully merging a pull request may close this issue.

2 participants