-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Make the translation function fully type-safe #1193
Conversation
This is an amazing usage of TS 4.1, hope to be able to use it as part of the library! |
That's great! I'd love the see this PR merged 🙏 |
@karol-majewski as you created this issue can you have a look at it? |
I don't know a ton about this library, and I'll admit I don't really understand the implementation, but before merging I'd encourage a bit of testing on the "worst-case" scenarios. You don't want to launch a ton of work on every keystroke. Does editing with a lot of fields become slow? Do compilation times increase by a decent amount if you add 1000 fields? |
Hey @DanielRosenwasser, thank you for taking some time to look at my PR and express your opinion, that's a really good point. I've performed some tests on the project that I've been working on, which has close to 2000 keys under the same namespace (legacy), and other hundreds of keys under different namespaces, and I didn't notice any significant increase in terms of time compilation, memory consumption and CPU. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Well it'll be relative. If it's no slower than today, that might be okay; however, we really do aim for less than half a second on completions. Part of me really wishes the speed tests we have in Definitely Typed were more easily runnable to get a sense of regressions here. |
Hey guys, I've created this repository https://github.com/pedrodurek/i18n-type-tests that contains examples of the "worst-case" scenarios, I encourage everyone who is reviewing this PR to take a look at it. In a nutshell, the timing to bring the keys considering that ns1 & ns2 have 2548 keys combined takes approximately 0.5-1 sec. Once we import ns3 as well, the total number of keys is 7647 and it's taking 2-3 sec mostly. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Hey, wondering what's missing for this PR, anything I can help out? Really wanted to use this on my projects with TS 4.1 |
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.
This looks pretty good, I didn't see any performance decrease even with large objects
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.
Tested on my side-projects and it handles really well!
If any help or review is needed, I am down to help 🎉 |
Thanks @tigerabrodi, I'd love some help reviewing it 🤗 |
@pedrodurek Will block out a good 30 - 60 minutes today, I think I myself can learn a thing or two, thanks for allowing me 💕 🥰 |
Just let me know when you think this PR is ready to be merged and rolled out... |
As mentioned, I will give it a review later (in 4-5h), really looking forward to contributing and working with you guys on react-i18next 🥰 |
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.
LGTM.
Just few comments, could also be because of my lack of experience/knowledge.
Overall, I would point out, we should try to avoid any
type as much as possible, but I bet you already know this 👍 💕 🎉
src/ts4.1/index.d.ts
Outdated
*/ | ||
export interface Resources {} | ||
|
||
type Omit<T, K> = Pick<T, Exclude<keyof T, K>>; |
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 think type Omit is already included in TypeScript.
Can be found here: https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys
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.
Good catch, this is coming from the original code, but it won't hurt if I remove it.
export function setI18n(instance: i18n): void; | ||
export function getI18n(): i18n; | ||
export const initReactI18next: ThirdPartyModule; | ||
export function composeInitialProps(ForComponent: any): (ctx: unknown) => Promise<any>; |
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 have a hard time understanding this function, it takes a component as a parameter, returns a context of unknown
type, which returns a promise of any
type?
ForComponent
is of typeany
? Could we be more specific of which type this is, maybeReactNode | ReactElement
?- Same as above, but with the Promise, is there a possibilty for us to be more specific about the type, as
any
type should be avoided as much as possible.
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.
Those types are coming from the original code, so I don't know much about the composeInitialProps
function, but I definitely agree, we shouldn't be using any, so we could create an issue to tackle that later.
src/ts4.1/index.d.ts
Outdated
export interface TransProps<N extends Namespace, K extends TFuncKey<N>, E extends Element = HTMLDivElement> | ||
extends React.HTMLProps<E> { | ||
children?: React.ReactNode; | ||
components?: readonly React.ReactNode[] | { [tagName: string]: 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.
Should readonly
not be placed before components
, like this:
readonly components?: React.ReactNode[] | { [tagName: string]: 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.
Again, this is coming from the original code, but you're definetly right, readonly won't have any effect if placed after.
Thank you @tigerabrodi, I really appreciate the rapid review! You're completely right, we should always avoid using any, unless you're using it for manipulation reasons, for example: type Test<T> = T extends any[] ? string : number; or like this, where we're omitting any array from T: type OmitArrayProps<T> = Exclude<T, keyof any[]>; This PR aims to make the Here we can see a comparison of the changes that I've made: https://gist.github.com/pedrodurek/b23c17bebd9619b0dc0651bc3949a82e/revisions#diff-7aa4473ede4abd9ec099e87fec67fd57afafaf39e05d493ab4533acc38547eb8 |
I'm happy to report that it works perfectly on a codebase with multiple namespaces and ~115 translated phrases. For anyone storing translations as JSON files just like me: your declaration can be as simple as: declare module 'react-i18next' {
interface Resources {
foo: typeof import('path/to/foo.json');
bar: typeof import('path/to/bar.json');
}
}
export {}; This requires |
How do you handle dynamic keys with this change? Example: // resources
const resourcesEN = {
variants: {
'a': 'text a',
'b': 'text b',
},
}
// component
const variant = 'a' as 'a' | 'b'
t(`variants.${variant}`) // fails EDIT: I have found a solution 😅 The execution should be: t(`variants.${variant}` as const) // passes |
Glad to see more people testing this out and checking the many opportunities, should we maybe have a section on the documentation for TypeScript 4.1 with all of these different solutions? |
For us this PR introduced a breaking change on minor version, as after this minor upgrade our code stopped building — every translation key is now type checked, and previously it wasn't 😬 I am now sitting a third hour trying to get to green again 😅
To be honest this feels like a big missing thing. Using Typescript with this package is definitely not easy and there is a lot of guess work involved. Having some documentation on this would be ideal! If someone starts some work on this, I'd gladly contribute 👍 |
I hadn't looked at that point, you're right. I'll take a look and start helping with some documentation. |
Just for reference i18next/i18next#1504 (comment) |
Is there a better solution than casting to const everywhere? |
Hey @joelpoloney, unfortunately that's the only way. That's a typescript behaviour ("deficiency"), when we concatenate strings, typescript automatically infers the type as string, and in order to get the literal values, we must assert the value. Take a look at this example. |
@pedrodurek ah interesting. I'd have thought TypeScript would be a bit smarter around this. Thank you for the explanation! |
I apologise for the negativity here, as I know how much effort it is to maintain an open source library, and I must say that react-i18next is a pleasure to use. Please keep up the good work. That being said, I just wanted to add a bit of feedback here, in that upgrading from 11.7 -> 11.8 has broken my codebase, in a way that I'm not sure how to fix without a decent amount of effort. I am probably using the library incorrectly, specifically around the typings, but regardless I wouldn't expect a minor jump to cause breaking changes. IMHO, this change should have been reserved for a major bump. I've added some context below. Here is an excerpt from my codebase: // views.json
{
"reviews": {
"connectPermissions": {
"facebook": {
"heading": "About to Connect Facebook",
"lead": "In a moment, you might be asked by Facebook if you are happy to give permission to:",
"permissionsList": [
{
"permission": "Show a list of Pages you manage",
"reason": "So you can select the page you want to connect."
},
{
"permission": "Read content posted on the Page",
"reason": "Allowing us show you a list of all your ratings."
}
]
},
"google": {
"heading": "About to Connect Google",
"lead": "In a moment, you'll be asked by Google if you are happy to give permission to:",
"permissionsList": [
{
"permission": "See, edit, create and delete your Google business listings ",
"reason": "We will only use this access to see the list of your businesses, notify you of new reviews, and allow you to reply to any review you receive."
}
]
}
}
}
}
``` ts
// use-scoped-translation.ts
import { StringMap, TFunctionResult, TOptions } from 'i18next';
import { useTranslation } from 'react-i18next';
/**
* Produces a 'scoped' translate function, eliminating the need to specify the entire key for a section of strings.
*
* To use, ensure the translation strings are all under a single parent key in the `views.json` locale file.
* Then, call this function with the key in i18n notation.
* The result is a t function which is scoped to the key in question.
*
* @param prefix i18n key prefix(es). Pass multiple prefixes to use i18next's built-in fallback mechanism.
*/
export default function useScopedTranslation(...prefixes: string[]) {
const { t: translate } = useTranslation();
function t<TResult extends TFunctionResult = string>(
key: string,
options?: TOptions<StringMap> | string
) {
const keys = prefixes.map((p) => `views:${p}.${key}`);
return translate<TResult>(keys, options);
}
return { t };
}
// connect-permissions.tsx
interface Props {
platform: 'google' | 'facebook';
}
interface PlatformStrings {
heading: string;
lead: string;
permissionsList: Array<{
permission: string;
reason: string;
}>;
}
const ReviewsPlatformConnectPermissionsPage: FC<Props> = ({
platform,
}) => {
const { t } = useScopedTranslation('reviews.connectPermissions');
const {
heading,
lead,
permissionsList,
permissionsNecessaryWarning,
} = t<PlatformStrings>(platform, { returnObjects: true });
return (
// ... JSX including the strings from above
);
}; As you can see, I'm dynamically fetching the correct strings according to the The problem I have when upgrading to 11.8 is that the |
@fiznool I'm sorry about that. We try to follow semver as good as possible...I should have consult @pedrodurek regarding the version impact before publishing. Will try to improve on that in future. |
Hey @fiznool, sorry about that, we had to switch the generic types because optional type parameters must always come first, and the return translate<string, TResult>(keys, options); If you want to type all keys and return type from |
Hey @joelpoloney and @DCzajkowski, this is going to be fixed on typescript 4.2. Take a look at the example here. It's still in beta version, but it's intended to be released on February. |
Ah, good to know. Thank you for the update! |
Thanks for the amazing work on this. It seems to work perfectly well! I do have an issue concerning passing translations dynamically, though. interface PropsType {
myTranslation: string // can I use an exposed type here??? string won't work
}
const MyComponent = (props: PropsType) => {
return (
<div>
{t(props.myTranslation)}
</div>
);
}; |
Hey @pierpo, thanks for your feedback! interface PropsType {
myTranslation: TFuncKey<'ns1'> // In case you have only one namespace, just pass 'translation'
} Alternatively, you can pass the actual literal values accepted. Like this: interface PropsType {
myTranslation: 'key.name' | 'key.description';
} Or something like: interface PropsType {
myTranslation: 'name' | 'description';
}
...
{t(`key.${props.myTranslation}` as const)} |
Thanks for the super quick answer 😊
Thanks again for the amazing work. I've been waiting for this feature for years! |
Is there any example out there showing how to use I see some potential to provide the mapping type directly to the
and that's the only option currently available, right? Can we expose a similar augmentation way to type the params? My point is to make sure that
|
Hey @ertrzyiks, right now, we don't support type safety for the interpolation params, but that's something that I was considering since the very beginning. The reason for not implementing is because typescript doesn't support getting literal values from JSON files yet microsoft/TypeScript#32063, but I'll keep you posted once it does. |
@pedrodurek I'm also interested in type safe interpolations. Regarding the issue you linked, there's a workaround for it possible, by transforming the json file into a
Edit: Ah, I see. I'd assume it would parsed out, in a manner similar to this comment: microsoft/TypeScript#32063 (comment) |
With Typescript 4.1, now it is possible to use recursive conditional type and template literal string. This PR aims to make the
t
function fully typed and supporting single and multiple namespaces. It works withuseTranslation (hooks)
,withTranslation (HoC)
,Trans Component
andTranslation (render prop)
.How to use it
You just have to rely on type augmentation technique to override the default Resources with the resource language object.
For example:
Type augmentation
And that's all!!! it works like magic!
I've prepared a few examples using
useTranslation
.Using single and multiple namespaces:
https://tsplay.dev/gWozjm
Array case within the resources JSON:
https://tsplay.dev/nmq0ZN
Without Namespace:
https://tsplay.dev/rw21xW
Below is the diff of the changes that I made:
https://gist.github.com/pedrodurek/b23c17bebd9619b0dc0651bc3949a82e/revisions#diff-7aa4473ede4abd9ec099e87fec67fd57afafaf39e05d493ab4533acc38547eb8
Let me know if you want me to change anything, thank you guys! I hope this can help everyone who relies on i18next and typescript.
Checklist
npm run test