Improve translations - flatten translation objects #25846
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
melvin-bot
bot
requested review from
narefyev91
and removed request for
a team
|
@narefyev91 Please 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] |
Reviewer Checklist
Screenshots/VideosWebweb3.movMobile Web - Chromeandroid-web3.movMobile Web - Safariios-web3.movDesktopdesktop3.moviOSios3.movAndroidandroid3.mov |
mountiny
changed the title
|
@narefyev91 @mountiny This might require another review, since I had to merge with main to fix conflicts and it forced me to port the new functions and translation object to typescript. |
| */ | ||
| // Necessary to export so that it is accessible to the unit tests | ||
| // eslint-disable-next-line rulesdir/no-inline-named-export | ||
| export function flattenObject(obj: Translation): TranslationFlatObject { |
There was a problem hiding this comment.
TranslationFlatObject is way too generic (along with Translation & TranslationBaseValue types). The problem is that we lose type safety using flattenObject function:
const translations = flattenObject(en);
const text2 = translations.nonExistingKey; // TranslationFlatObject, meanwhile it's undefined in realityThis is a big problem as we should infer what keys are available to stay type safe when using Localize inside of components.
What we should do is to flatten the object in Typescript. Something like this (link):
type Join<K, P> = K extends string | number ? (P extends string | number ? `${K}${'' extends P ? '' : '.'}${P}` : never) : never;
type Prev = [never, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, ...Array<0>];
type Leaves<T, D extends number = 10> = [D] extends [never] ? never : T extends object ? {[K in keyof T]-?: Join<K, Leaves<T[K], Prev[D]>>}[keyof T] : '';
declare function flattenObject<TTranslation extends Translation>(obj: TTranslation): Record<Leaves<TTranslation>, string | ...>;
const enFlattened = flattenObject2(en);There was a problem hiding this comment.
However this will be quite irritating to use inside of components as the value is not inferred and it's too generic: string | string[] | Function. So we would have to always check the types inside of components 😒
There was a problem hiding this comment.
Actually I had a similar problem in my side project, something like this could work here but the type is quite complicated ngl 😅 (this would have to be tested as I used it a little bit differentl in my project)
// Flatten object type
type FlattenObject<T> = CollapseEntries<CreateObjectEntries<T, T>>;
type Entry = {key: string; value: unknown};
type EmptyEntry<T> = {key: ''; value: T};
// Transforms entries to one flattened type
type CollapseEntries<T extends Entry> = {
[E in T as E['key']]: E['value'];
} extends infer V
? {[K in keyof V]: V[K]}
: never;
type CreateObjectEntries<T, I> = T extends infer U
? // Checks that U is an object
U extends object
? {
// Checks that Key is of type string
[K in keyof U]-?: K extends string
? // Nested key can be an object, run recursively to the bottom
CreateObjectEntries<U[K], I> extends infer E
? E extends Entry
? {
key: E['key'] extends '' ? K : `${K}.${E['key']}`;
value: E['value'];
}
: never
: never
: never;
}[keyof U] // Builds entry for each key
: EmptyEntry<U>
: never;
declare function flattenObject2<TTranslation extends Translation>(obj: TTranslation): FlattenObject<TTranslation>;
const enFlattened = flattenObject2(es);
const type = enFlattened['common.websiteExample']; // stringThere was a problem hiding this comment.
Also cc @fabioh8010 as this is quite important to get right 😅
|
@BeeMargarida what do you think of the above, my types game is not that great yet 😂 |
Currently working on this. My typing skills are not at this level 😬, but I'm trying to wrap my head around it and craft a solution for this. |
mountiny
changed the title
|
@mountiny @blazejkustra Updated! Types for the flat object should now be more robust (thank you @fabioh8010 🙌). |
| @@ -190,7 +192,41 @@ type RemovedTheRequestParams = {valueName: string; oldValueToDisplay: string}; | |||
|
|
|||
| type UpdatedTheRequestParams = {valueName: string; newValueToDisplay: string; oldValueToDisplay: string}; | |||
|
|
|||
| /* Translation Object types */ | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| type TranslationBaseValue = string | string[] | ((...args: any[]) => string); | |||
There was a problem hiding this comment.
@blazejkustra We need any[] here I think, I tried with unknown[] and every function starts failing.
|
Updated! |
|
@mountiny re-tested again - looking good! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.69-0 🚀
|
|
@BeeMargarida @narefyev91 @mountiny I'm just curious, why do we need to flatten translation objects? maybe for performance? |
|
👍 |
|
PR to fix regressions: #27358 |
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.69-2 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.70-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.70-8 🚀
|
Details
Localize.translateto use this new object (removelodashGetusages)Performance improvement of ±50%.
Fixed Issues
$ #25466
Tests
Offline tests
QA Steps
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
Web
web_t.mp4
Mobile Web - Chrome
mchrome_t.mp4
Mobile Web - Safari
msafari_t.mp4
Desktop
desktop_t.mp4
iOS
ios_t.mp4
Android
android_t.mp4