[TS migration] Migrate useLocalize#29180
Conversation
| const Provider = compose( | ||
| withCurrentUserPersonalDetails, | ||
| withOnyx({ | ||
| preferredLocale: { | ||
| key: ONYXKEYS.NVP_PREFERRED_LOCALE, | ||
| selector: (preferredLocale) => preferredLocale, | ||
| }, | ||
| }), | ||
| )(LocaleContextProvider); |
There was a problem hiding this comment.
Change to:
| const Provider = compose( | |
| withCurrentUserPersonalDetails, | |
| withOnyx({ | |
| preferredLocale: { | |
| key: ONYXKEYS.NVP_PREFERRED_LOCALE, | |
| selector: (preferredLocale) => preferredLocale, | |
| }, | |
| }), | |
| )(LocaleContextProvider); | |
| const Provider = compose( | |
| withCurrentUserPersonalDetails, | |
| withOnyx<LocaleContextProviderProps, LocaleContextProviderOnyxProps>({ | |
| preferredLocale: { | |
| key: ONYXKEYS.NVP_PREFERRED_LOCALE, | |
| selector: (preferredLocale) => preferredLocale ?? CONST.LOCALES.DEFAULT, | |
| }, | |
| }), | |
| )(LocaleContextProvider); |
| type LocaleContextProviderProps = { | ||
| /** The user's preferred locale e.g. 'en', 'es-ES' */ | ||
| preferredLocale: string; | ||
|
|
||
| /** The current user's personalDetails */ | ||
| currentUserPersonalDetails: CurrentUserPersonalDetails; | ||
|
|
||
| /** Actual content wrapped by this component */ | ||
| children: React.ReactNode; | ||
| }; |
There was a problem hiding this comment.
Split into two:
| type LocaleContextProviderProps = { | |
| /** The user's preferred locale e.g. 'en', 'es-ES' */ | |
| preferredLocale: string; | |
| /** The current user's personalDetails */ | |
| currentUserPersonalDetails: CurrentUserPersonalDetails; | |
| /** Actual content wrapped by this component */ | |
| children: React.ReactNode; | |
| }; | |
| type LocaleContextProviderOnyxProps = { | |
| /** The user's preferred locale e.g. 'en', 'es-ES' */ | |
| preferredLocale: string; | |
| }; | |
| type LocaleContextProviderProps = LocaleContextProviderOnyxProps & { | |
| /** The current user's personalDetails */ | |
| currentUserPersonalDetails: CurrentUserPersonalDetails; | |
| /** Actual content wrapped by this component */ | |
| children: React.ReactNode; | |
| }; |
| formatPhoneNumber: () => '', | ||
| toLocaleDigit: () => '', | ||
| fromLocaleDigit: () => '', | ||
| preferredLocale: '', |
There was a problem hiding this comment.
Use CONST.LOCALES.DEFAULT as a default
MaciejSWM
changed the title
| translate: Translate; | ||
| numberFormat: NumberFormat; | ||
| datetimeToRelative: DatetimeToRelative; | ||
| datetimeToCalendarTime: DatetimeToCalendarTime; | ||
| updateLocale: UpdateLocale; | ||
| formatPhoneNumber: FormatPhoneNumber; | ||
| toLocaleDigit: ToLocaleDigit; | ||
| fromLocaleDigit: FromLocaleDigit; | ||
| preferredLocale: string; |
There was a problem hiding this comment.
@MaciejSWM Can you add comments to describe these props?
/** Returns translated string for given locale and phrase */
translate: type
/** Formats number formatted according to locale and options */
numberFormat: type
/** Converts a datetime into a localized string representation that's relative to current moment in time */
datetimeToRelative: type
/** Formats a datetime to local date and time string */
datetimeToCalendarTime: type
/** Updates date-fns internal locale */
updateLocale: type
/** Returns a locally converted phone number for numbers from the same region
* and an internationally converted phone number with the country code for numbers from other regions */
formatPhoneNumber: type
/** Gets the standard digit corresponding to a locale digit */
fromLocaleDigit: type
/** Gets the locale digit corresponding to a standard digit */
toLocaleDigit: type|
|
||
| type LocaleContextProviderOnyxProps = { | ||
| /** The user's preferred locale e.g. 'en', 'es-ES' */ | ||
| preferredLocale: string; |
There was a problem hiding this comment.
I would suggest doing something like this
| preferredLocale: string; | |
| preferredLocale: ValueOf<typeof CONST.LOCALES>; |
|
|
||
| type SelectedTimezone = { | ||
| /** Value of the selected timezone */ | ||
| selected: string; |
There was a problem hiding this comment.
Maybe use timezones from TIMEZONES.js to make it more strict?
|
|
||
| const translate = useMemo<Translate>(() => (phrase, variables) => Localize.translate(preferredLocale, phrase, variables), [preferredLocale]); | ||
|
|
||
| const numberFormat = useMemo<NumberFormat>(() => (number, options) => NumberFormatUtils.format(preferredLocale, number, options), [preferredLocale]); |
There was a problem hiding this comment.
I noticed that some of these util functions (lines 95-116) accept strings, this is something we haven't noticed before, so I'd recommend going through all of them and making sure that they are strict enough - in cases where we use timezones, make use of that new type, and where we use LOCALES make use of ValueOf
| children: React.ReactNode; | ||
| }; | ||
|
|
||
| type Translate = (phrase: string, variables: Record<string, string>) => string; |
There was a problem hiding this comment.
@MaciejSWM let's make this type smarter, here is my idea:
| type Translate = (phrase: string, variables: Record<string, string>) => string; | |
| type Translate = <TKey extends TranslationPaths>( | |
| phrase: TKey, | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| variables?: TranslateType<EnglishTranslation, TKey> extends (...args: any) => any ? Parameters<TranslateType<EnglishTranslation, TKey>>[0] : never, | |
| ) => string; |
This type will allow us to only use the keys that exist and, when is a key that needs variables, we will have type inference too.
translate('addDebitCardPage.nameOnCard');
translate('chronos.oooEventSummaryFullDay', {
summary: '',
dayCount: 1,
date: '',
});Note: You will have to export TranslateType and TranslationPaths types from src/languages/types.ts.
There was a problem hiding this comment.
@fabioh8010 We implemented a similar approach in this PR (src/libs/Localize/index.ts, translate function).
type PhraseParameters<T> = T extends (...args: infer A) => string ? A : never[];
type Phrase<TKey extends TranslationPaths> = TranslationFlatObject[TKey] extends (...args: infer A) => unknown ? (...args: A) => string : string;
function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' | 'es-ES' | 'es_ES', phraseKey: TKey, ...phraseParameters: PhraseParameters<Phrase<TKey>>): string {☝️ Localize.transalate is used in LocaleContextProvider.tsx, so types should be the same in both places - let's decide on one
edit: just updated Localize PR, it's ready for cross review 👍
There was a problem hiding this comment.
@MaciejSWM @blazejkustra The way it was implemented in that PR works better, let's use it then!
| type SelectedTimezone = { | ||
| /** Value of the selected timezone */ | ||
| selected: (typeof TIMEZONES)[number]; | ||
| }; | ||
|
|
||
| type CurrentUserPersonalDetails = { | ||
| /** Timezone of the current user */ | ||
| timezone?: SelectedTimezone; | ||
| }; |
There was a problem hiding this comment.
| type SelectedTimezone = { | |
| /** Value of the selected timezone */ | |
| selected: (typeof TIMEZONES)[number]; | |
| }; | |
| type CurrentUserPersonalDetails = { | |
| /** Timezone of the current user */ | |
| timezone?: SelectedTimezone; | |
| }; | |
| type CurrentUserPersonalDetails = Pick<PersonalDetails, 'timezone'>; |
Shouldn't this be easier?
|
Thanks @fabioh8010 ! I implemented your suggestions and it's ready for re-review |
|
TS failing. I'm looking for a fix |
|
Tests are failing 😢 |
|
@thesahindia @ One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@thesahindia We would appreciate if you could work on the reviewer checklist this week as this PR is blocking other migration PRs, thanks! |
|
Taking this one over! |
|
|
||
| type UpdateLocale = () => void; | ||
|
|
||
| type FormatPhoneNumber = (phoneNumber: string) => string; |
There was a problem hiding this comment.
@blazejkustra Defining types for each function like this seems a little redundant to me. Is this a general pattern we're using throughout our app? I would just add these typings when defining the function.
There was a problem hiding this comment.
All these types are used in LocaleContextProps type and their respective implementation in useMemos. I agree it looks rather clunky, I refactored the code a bit, wdyt? cc @BartoszGrajdek / @kubabutkiewicz
|
|
||
| const fromLocaleDigit = useMemo<FromLocaleDigit>(() => (localeDigit) => LocaleDigitUtils.fromLocaleDigit(locale, localeDigit), [locale]); | ||
|
|
||
| const contextValue = useMemo( |
There was a problem hiding this comment.
Did you miss to type this?
There was a problem hiding this comment.
Yep, good catch. I fixed it!
| import {useContext} from 'react'; | ||
| import {LocaleContext, LocaleContextProps} from '@components/LocaleContextProvider'; | ||
|
|
||
| export default function useLocalize(): LocaleContextProps { |
There was a problem hiding this comment.
Typing this is redundant. I think the type would be inferred here. No?
There was a problem hiding this comment.
Yes, but specifying return types aligns with TS guidelines. We decided to almost always specify return types for two reasons:
- Better visibility
- Improved TS performance when type checking
| import CONST from '@src/CONST'; | ||
| import * as NumberFormatUtils from './NumberFormatUtils'; | ||
|
|
||
| type Locale = ValueOf<typeof CONST.LOCALES>; |
There was a problem hiding this comment.
I see this being repeated at other places as well. How about moving this to the CONST file as well?
There was a problem hiding this comment.
We decided to for now keep it in the files that uses these CONST types, there will be a follow up task that cleans libs/hooks/HOCs and we will definitely move such types to a different file.
|
@allroundexperts Answered your questions and adjusted the code, thank you for your input on these! |
|
Friendly buump @allroundexperts |
|
@allroundexperts kind bump 😄 |
|
I was just finishing up the screenshots 😄 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2023-11-13.at.3.15.11.PM.movAndroid: mWeb ChromeScreen.Recording.2023-11-13.at.2.36.08.PM.moviOS: NativeScreen.Recording.2023-11-13.at.2.35.03.PM.moviOS: mWeb SafariScreen.Recording.2023-11-13.at.2.32.15.PM.movMacOS: Chrome / SafariScreen.Recording.2023-11-13.at.2.26.41.PM.movMacOS: DesktopScreen.Recording.2023-11-13.at.2.30.21.PM.mov |
mountiny
changed the title
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Changes are looking good to me, lets be on a look out for any date related regressions maybe also with the datepicker
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.99-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.99-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.0-0 🚀
|
8 participants
Details
Fixed Issues
$ #24940
PROPOSAL:
Tests
Offline tests
TestsQA Steps
Use the date picker to set the date of brith in personal details, verify it works as expected and selected values are correct.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2023-10-10.at.14.25.51.mov
Android: mWeb Chrome
Screen.Recording.2023-10-10.at.14.27.58.mov
iOS: Native
Screen.Recording.2023-10-10.at.14.04.35.mov
iOS: mWeb Safari
Screen.Recording.2023-10-10.at.14.05.40.mov
MacOS: Chrome / Safari
Screen.Recording.2023-10-10.at.14.07.07.mov
MacOS: Desktop
Screen.Recording.2023-10-10.at.14.01.43.mov