-
-
Notifications
You must be signed in to change notification settings - Fork 217
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: Add type safety for timeZone parameter #359
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@A7med3bdulBaset is attempting to deploy a commit to the next-intl Team on Vercel. A member of the Team first needs to authorize it. |
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.
Hey and many thanks for this PR! This is a great DX improvement! 🙌
I've added a few nits, also I think CI isn't passing yet. I've just approved the main
workflow, please have a look in regard to failed linting and type checking!
| "UTC" | ||
| "W-SU" | ||
| "WET" | ||
| "Zulu"; |
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.
I'm wondering if we should allow unknown strings here too (see this trick).
The benefit of allowing unknown strings is that we don't have to publish a new version in case our list gets outdated. This might even be critical for users on outdated versions, if we've released breaking changes in the meantime, otherwise we'd have to backport changed time zones. AFAIK React does this too, e.g. for ARIA roles.
The downside is of course that we don't fail for invalid time zones.
However, JavaScript runtimes throw an error for unknown time zones:
(I've supplied "Unknown" in this example)
Therefore, I'd say we should support unknown strings.
import useIntlContext from './useIntlContext'; | ||
|
||
export default function useTimeZone() { | ||
export default function useTimeZone(): TimeZone { |
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 return type is wrong, also undefined
is allowed. Better just let TypeScript infer the return type and don't annotate it additionally!
TODOs:
|
Since You are using spaces for indentation not tabs, and no space inside curly braces, I think this is the best way to go. In general, It's OPEN-SOURCE, so We have to have a consistent config file
🔴 Move out of utils 🔴 Use default export/import 🔴 Rename file to be singular 🔴 Allow unknown strings
- Change import to default - fix linting errors
- Change import to default - Edit import path to exclude `utils` - fix linting errors
- Change import to default - Edit import path to exclude `utils` - fix linting errors
- Change import to default - Edit import path to exclude `utils` - fix linting errors
All Done and Ready for Review I ran locally pnpm build && pnpm lint && pnpm test and all tests passed. |
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.
Looks fantastic, thank you so much! 🙌
I've left one minor note about the prettier configs. If you could remove them, we can merge this PR!
Looks great, will be released in a minute! Are you interested in adopting this in the RSC support branch as well (e.g. |
WIP |
This PR introduces type safety for the
timeZone
parameter instead of usingstring
as the type. It enhances the developer experience by ensuring that only valid time zone values are accepted and providing better code completion and type-checking support.Changes Made:
TimeZone
type that is a union of all valid time zone values in/use-intl/src/core/utils/TimeZones.tsx
timeZone
parameter in these files:IntlConfig
DateTimeFormatOptions
createFormatter
convertFormatsToIntlMessageFormats
Data Source:
Time zone values sourced from tz database.