-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[TS migration] Migrate useLocalize #29180
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use CONST.LOCALES.DEFAULT
as a default
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you miss to type this?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing this is redundant. I think the type would be inferred here. No?
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.
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 * as NumberFormatUtils from './NumberFormatUtils'; | ||
|
||
type Locale = ValueOf<typeof CONST.LOCALES>; |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 good!
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.
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 🚀
|
Details
Fixed Issues
$ #24940
PROPOSAL:
Tests
Offline tests
Tests
QA 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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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