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

feat: Typesafe Global Formats #1346

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

dBianchii
Copy link

@dBianchii dBianchii commented Sep 18, 2024

This PR adds a new configuration called IntlFormats, where users can link their global formatting to this interface, and allow for global formats typesafety to work across their app.

TODO

  • Make the type fallback to string if it was not provided
  • Adapt existing examples to use this new feature
  • Add this to the docs
  • Create tests (?)

Closes #1112

Copy link

vercel bot commented Sep 18, 2024

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

Name Status Preview Comments Updated (UTC)
next-intl-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 21, 2024 1:51pm
next-intl-example-app-router ❌ Failed (Inspect) Sep 21, 2024 1:51pm
next-intl-example-app-router-without-i18n-routing ❌ Failed (Inspect) Sep 21, 2024 1:51pm

Copy link

vercel bot commented Sep 18, 2024

@dBianchii is attempting to deploy a commit to the next-intl Team on Vercel.

A member of the Team first needs to authorize it.

@dBianchii
Copy link
Author

dBianchii commented Sep 18, 2024

Hi @ amann/maintainers

You said in here that the package supports full, long, medium and short. I believe that this only applies for dates and times within messages, yes? In that comment, did you suggest to make those the defaults as well for the formatter?

You also said:

For number formatting there are defaults for currency and percent.

I haven't found this logic in the code, and also in my tests when I do format.number(5, "percent") or format.number(5, "currency"), nothing happens

Could you explain a bit more of what you meant?

EDIT: Looks like that the percent and currency are available for formatting within messages as well! Got it. In that case, I just need to understand how you think that these should relate to the formatter api.

Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Damn you're quick! 🔥🔥

I somehow left this feature on the backlog for a long time, but you made it happen just over night! 😄 Super cool, thanks a lot!

I've left a few comments inline, let me know what you think!

Btw. regarding the defaults across usage in messages / via format.*: You're absolutely right, I've opened #1347 as a separate issue to discuss this. This is definitely out of scope for this PR and type-safety is only relevant for format.* calls, therefore we should assume no defaults there.

examples/example-app-router/src/i18n/request.ts Outdated Show resolved Hide resolved
packages/use-intl/src/core/createFormatter.tsx Outdated Show resolved Hide resolved
packages/use-intl/types/index.d.ts Outdated Show resolved Hide resolved
examples/example-app-router/src/i18n/request.ts Outdated Show resolved Hide resolved
examples/example-app-router/src/i18n/request.ts Outdated Show resolved Hide resolved
packages/use-intl/src/core/createFormatter.tsx Outdated Show resolved Hide resolved
@dBianchii
Copy link
Author

There is one problem with this right now:

I see that the global format api has been designed to be able to handle global formats for client and server differently/separately. (We must provide formats to both getRequestConfig and NextIntlClientProvider component. Each can take in different global formats. ) I don't really understand why it is this way, because in my opinion it would be better if we could just use the same global formats everywhere (client and server). The way I set this up makes it impossible to separate both types, and the user can only provide one type as IntlFormats.

How can we resolve this problem?

@amannn
Copy link
Owner

amannn commented Sep 19, 2024

it would be better if we could just use the same global formats everywhere (client and server)

Yep, I agree!

The discussion is related to the one about messages: Messages are defined globally, but it's the user's job to provide the relevant messages where relevant. This is to allow the user to optionally pass only certain messages to the client side to optimize bundle size, which is also the reason why the messages need to be passed explicitly to NextIntlClientProvider.

The current state of the library has the same mechanism for formats: In order to potentially reduce bundle size, formats aren't automatically inherited and need to be passed explicitly to NextIntlClientProvider. I've changed my mind in the meantime about this though: Out-of-the-box formats should be inherited automatically, and therefore the user doesn't have to worry. You could still opt-out with formats={null}. This change is already implemented on the v4 branch (see #1191), but the release is still a bit further down the road.

That being said, I think it's fine to model IntlFormats the same as the global IntlMessages, especially with the change in v4 coming up.

Hope that sounds reasonable to you!

@dBianchii
Copy link
Author

That's awesome to hear. Makes total sense to me. I'll continue with this decision in mind. Thanks!

@dBianchii
Copy link
Author

Ok, so I added a few tests to app-router-playground. Please let me know if I should add more. As I mentioned in the comment, I am only testing an environment that has the global IntlFormat type declared. Maybe if you'd like I could do it as well in another workspace.

Please let me know if you think that this implementation is good enough, or if there could be something missing/edge cases. Whenever you think all is good, I'll go ahead and start working on the docs. I have to say I'm very impressed with the documentation on this project. I'll do my best to follow suit and try to explain things in the best manner possible.

1. Add comment for type safe formats in `example-app-router-playground`
2. No need to execute type tests in `ClientContent`
3. A few more tests for `useFormatter` when no strict types are supplied
@amannn
Copy link
Owner

amannn commented Sep 20, 2024

Looks awesome, thanks a lot! I had another look over the changed files and cleaned up a few minor things along the way and added some more tests for the case when no strict formats are supplied: 3fa91e8. Hope that's ok for you!

I think the implementation is solid now! 💯

You wanna give the docs a shot? 🙂 Means a lot to me if you like them! 😊

@dBianchii
Copy link
Author

Ready to go. Please check though: I don't know why but I am getting some problems running pnpm i for this branch and also main branch.

image

I noticed I accidentally pushed a bunch pnpm lockfile changes, so I tried to use the correct pnpm version to revert it but no luck in running the installer.

Feel free to make any further changes

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.

Strict typing of global formats
2 participants