Skip to content
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

Add documentation for the new breaking change in TS #1852

Closed
DavidArmendariz opened this issue Oct 20, 2022 · 17 comments · Fixed by #1853
Closed

Add documentation for the new breaking change in TS #1852

DavidArmendariz opened this issue Oct 20, 2022 · 17 comments · Fixed by #1853

Comments

@DavidArmendariz
Copy link

Documentation issue

Yesterday, there was a breaking change where now the t function is not returning a string anymore. It is returning a DefaultTFuncReturn. This is breaking a lot of things in my React app because I used to say that t would just return a string. In fact, it still does so when you use variables inside the string like this i18next.t('{{var}} something', { var }).

Motivation

I have researched and there is no documentation on why this change was made or how it would impact existing projects.

@adrai
Copy link
Member

adrai commented Oct 20, 2022

@pedrodurek @rosskevin it starts 😉

@httpete
Copy link

httpete commented Oct 20, 2022

yes, seeing the same on this.

index.tsx:46:34 - error TS2769: No overload matches this call.
  Overload 1 of 2, '(props: { component: ElementType<any>; } & SystemProps<Theme> & { align?: "left" | "right" | "inherit" | "center" | "justify"; children?: ReactNode; ... 6 more ...; variantMapping?: Partial<...>; } & CommonProps & Omit<...>): Element', gave the following error.
    Type 'DefaultTFuncReturn' is not assignable to type 'ReactNode'.
      Type 'object' is not assignable to type 'ReactNode'.
  Overload 2 of 2, '(props: DefaultComponentProps<TypographyTypeMap<{}, "span">>): Element', gave the following error.
    Type 'DefaultTFuncReturn' is not assignable to type 'ReactNode'.

@httpete
Copy link

httpete commented Oct 20, 2022

I really wonder why this is done? This will be a tremendous change. What is the benefit?

@adrai
Copy link
Member

adrai commented Oct 20, 2022

@httpete
Copy link

httpete commented Oct 20, 2022

i18next has been the most stable api for so long of all the packages I use - I really hope that this change is making life better somehow.

@adrai
Copy link
Member

adrai commented Oct 20, 2022

i18next has been the most stable api for so long of all the packages I use - I really hope that this change is making life better somehow.

JS api is still the same... only the TS interface did receive a major rewrite because of many "wishes" by the community... nevertheless it's a semver major bump.

Please be patient and give the opportunity to @pedrodurek and @rosskevin to explain this TS change.

@pedrodurek
Copy link
Member

We simply moved the type enhancements from react-i18next to here. So we can also leverage t function fully type-safe from i18next, react-i18next and other libraries. That's the main motivation.
@httpete and @DavidArmendariz Just sharing the type error doesn't help much, could you please also provide an example. It can be a piece of code where the error is being thrown.

@pedrodurek
Copy link
Member

I was waiting to release both versions to create some examples of use. Later today, I'll put a PR up with some examples.

@DavidArmendariz
Copy link
Author

DavidArmendariz commented Oct 20, 2022

Well for example, I am using Material UI, and some props expect things to be string | undefined. If I pass a translated string to it, it will complain because the return type is DefaultTFuncReturn. And I don't know if casting the translation result to a string is the cleanest solution...

In general, if you are using any 3rd party library and their props expect strings, this will be a problem.

@httpete
Copy link

httpete commented Oct 20, 2022

yep, the first bitten part was:

   <Icon iconName="iconLoading.gif" ariaLabel={t('common:loading')} />
   
   
 /** Accessibility label */

ariaLabel?: string | undefined;

@httpete
Copy link

httpete commented Oct 21, 2022

I am also fiding in more harmless invocations:

   Type 'DefaultTFuncReturn' is not assignable to type 'ReactNode'.

46       <Typography sx={{ ml: 1 }}>{t('common:loading')}</Typography>

@pedrodurek
Copy link
Member

The PR that I just opened should fix the problem, but I really recommend to follow the steps here https://www.i18next.com/overview/typescript, to get the best of i18next and react-i18next.

@rosskevin
Copy link
Collaborator

rosskevin commented Oct 21, 2022

@httpete and @DavidArmendariz - in the future, the best way to convey your case would be to PR a minimal breaking test - e.g. check out https://github.com/i18next/i18next/blob/master/test/typescript/t.test.ts#L39

If it is react related, check out something like https://github.com/i18next/react-i18next/blob/master/test/typescript/useTranslation.test.tsx and PR one there.

For our codebase update (which included mui 4) on a large codebase, I had only three or so TFunction changes (cast related), and ironically enough it exposed several (10-15) mui title attributes that were NonNullable but were never reported as problems (and they were indeed problems)...so it seems like an improvement overall, but I agree the default value will make less friction (#1853).

By PR'ing minimal breaking tests, it allows us to not only fix these now, but make sure we don't have regressions later. Thanks for taking time to research the issue and trying out this major release.

@httpete
Copy link

httpete commented Oct 21, 2022

What I am finding here that makes all the problems go away is to:
1.) Update all packages for i18next and react-i18next to the latest.
2.) My current react-i18next.d.ts is exactly what follows this example: https://react.i18next.com/latest/typescript.
3.) Just change in your react-i18next.d.ts file:

declare module 'react-i18next' {

to:

declare module 'i18next' {

Everything is back to normal.

@httpete
Copy link

httpete commented Oct 21, 2022

@httpete and @DavidArmendariz - in the future, the best way to convey your case would be to PR a minimal breaking test - e.g. check out https://github.com/i18next/i18next/blob/master/test/typescript/t.test.ts#L39

If it is react related, check out something like https://github.com/i18next/react-i18next/blob/master/test/typescript/useTranslation.test.tsx and PR one there.

For our codebase update (which included mui 4) on a large codebase, I had only three or so TFunction changes (cast related), and ironically enough it exposed several (10-15) mui title attributes that were NonNullable but were never reported as problems (and they were indeed problems)...so it seems like an improvement overall, but I agree the default value will make less friction (#1853).

By PR'ing minimal breaking tests, it allows us to not only fix these now, but make sure we don't have regressions later. Thanks for taking time to research the issue and trying out this major release.

I also found @rosskevin , it found some legit bad usages in our codebase as well. Thank you for keeping our ts game tight. We recently did this with Zustand 4 as well, it seems that as TS gets better and developer understanding how to leverage it improves, it ripples through everything. This is good.

@DavidArmendariz
Copy link
Author

@rosskevin @pedrodurek @adrai Looks like the new version (22.0.3) re-introduced the same problem.

@rosskevin
Copy link
Collaborator

rosskevin commented Oct 27, 2022

@DavidArmendariz I haven't seen a problem today. I did see that my casts needed to be updated, as they were also incorrect based on the latest e.g. {returnObject: true}. https://github.com/i18next/i18next/blob/master/test/typescript/t.test.ts#L19-L22 (related to #1858). The only other change was https://github.com/i18next/i18next/pull/1860/files.

I suggest you open a new issue, link to this, and provide a test case. Or if it is react related, do so there. Breaking tests in PRs are very valuable and prevent regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants