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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

context values resolved differently (and incorrectly) between <Trans /> and t(...) #1730

Closed
stefan-schweiger opened this issue Mar 6, 2024 · 5 comments 路 Fixed by #1732
Closed
Assignees

Comments

@stefan-schweiger
Copy link

stefan-schweiger commented Mar 6, 2024

馃悰 Bug Report

<Trans /> and t(...) don't throw the same type errors when used with different context edge cases. But besides being inconsistent both <Trans /> and t(...) don't correctly interfere if the context is ok or not.

To Reproduce

https://stackblitz.com/edit/vitejs-vite-pabdwy?file=src%2FApp.tsx&terminal=dev

{
  "testContext1": "EN: {{context}}",
  "testContext1_Test1": "EN: Context1 Test1",
  "testContext1_Test2": "EN: Context1 Test2",
  "testContext2_Test1": "EN: Context2 Test1",
  "testContext2_Test2": "EN: Context2 Test2"
}
// WRONG: type error but SHOULD work
// Type '{ context: "asdf"; }' is not assignable to type 'string'
{t('testContext1', { context: 'asdf' })}
// OK: works
<Trans t={t} i18nKey="testContext1" context={'asdf'} />
// OK: works
<Trans t={t} i18nKey="testContext1" values={{ context: 'asdf' }} />

// OK: type error
// Type '{ context: "asdf"; }' is not assignable to type 'string'
{t('testContext2', { context: 'asdf' })}
// OK: type error
// Type '"testContext2"' is not assignable to type '"testContext2_Test1" | ...
<Trans t={t} i18nKey="testContext2" context={'asdf'} />
// OK: type error
// Type '"testContext2"' is not assignable to type '"testContext2_Test1" | ...
<Trans t={t} i18nKey="testContext2" values={{ context: 'asdf' }} />

// OK: works
{t('testContext2', { context: 'Test1' })}
// WRONG: type error but SHOULD work
// Type '"testContext2"' is not assignable to type '"testContext2_Test1" | ...
<Trans t={t} i18nKey="testContext2" context={'Test1'} />
// WRONG: type error but SHOULD work
// Type '"testContext2"' is not assignable to type '"testContext2_Test1" | ...
<Trans t={t} i18nKey="testContext2" values={{ context: 'Test1' }} />

Expected behavior

See output above.

@adrai
Copy link
Member

adrai commented Mar 6, 2024

@marcalexiei can you have a look at this?

@marcalexiei
Copy link
Member

// WRONG: type error but SHOULD work
// Type '{ context: "asdf"; }' is not assignable to type 'string'
{t('testContext1', { context: 'asdf' })}

Right now i18next context validation requires to pass a valid context which means that context must have 'Test1' | 'Test2' type if provided.

If you need to access default value do not provide context parameter
(or if context parameter is not valid on runtime it will fallback to the "default" context value)

image

I'm not familiar with the types of react-i18next so far but after a quick look I can see that <Trans /> values and context props are not typed with "strict" types as you can see here:

context?: string;

and here

Have them work properly it's not trivial as far as I can tell now 馃様.
I'll try to make a PR in the upcoming days but if anyone wants to try a fix feel free to open a PR.

@stefan-schweiger
Copy link
Author

stefan-schweiger commented Mar 6, 2024

But what if you want/need to provide a "unknown" context? For example imagine translating a long list of potential errors for which you get an error code from the backend. You have a somewhat good idea what the codes will be and you want to display them in the same location. But if you encounter a unknown error you want to fall back to the "generic" context.

{
   "error": "Unknown error, with error code {{context}}. Please contact support.",
   "error_NotFound": "The resource you are looking for could not be found.",
   "error_Disabled": "Can't edit the resource because it's disabled.",
   // ...
}
const Error = ({ errorCode: string }) => {
    const { t } = useTranslation();
    
    return <span>{t('error', { context: errorCode })}</span>
}

I know that you can try to workaround this issue in different ways, but in theory this is already supported by i18next, just not reflected in the typings.

Do you see any way of getting such cases handled correctly depending on if a key without a context exists or not?

If that is not possible what do you think about having at least an additional TypeOption like typedContext (or whatever makes sense) which if set to false in the CustomTypeOptions would ignore the context for the typings? I know that the implications are that if you don't have a "fallback" it will just display the key, but at least the type errors would be consistent.

@jamuhl
Copy link
Member

jamuhl commented Mar 6, 2024

fallback could be handled like https://www.i18next.com/principles/fallback#key-fallback

context is more for a known set of option

@stefan-schweiger
Copy link
Author

About the other issue with correctly typing the context, after tinkering around a bit this seems to work at least for all the cases we use in our application:

diff --git a/node_modules/react-i18next/TransWithoutContext.d.ts b/node_modules/react-i18next/TransWithoutContext.d.ts
index bf30d2a..c1350e2 100644
--- a/node_modules/react-i18next/TransWithoutContext.d.ts
+++ b/node_modules/react-i18next/TransWithoutContext.d.ts
@@ -1,4 +1,4 @@
-import type { i18n, ParseKeys, Namespace, TypeOptions, TOptions, TFunction } from 'i18next';
+import type { i18n, ParseKeys, Namespace, TypeOptions, TOptions, TFunction, _ContextSeparator } from 'i18next';
 import * as React from 'react';
 
 type _DefaultNamespace = TypeOptions['defaultNS'];
@@ -10,11 +10,12 @@ export type TransProps<
   TOpt extends TOptions = {},
   KPrefix = undefined,
   E = React.HTMLProps<HTMLDivElement>,
+  TContext extends string | undefined = undefined
 > = E & {
   children?: TransChild | readonly TransChild[];
   components?: readonly React.ReactElement[] | { readonly [tagName: string]: React.ReactElement };
   count?: number;
-  context?: string;
+  context?: TContext;
   defaults?: string;
   i18n?: i18n;
   i18nKey?: Key | Key[];
@@ -29,7 +30,8 @@ export type TransProps<
 export function Trans<
   Key extends ParseKeys<Ns, TOpt, KPrefix>,
   Ns extends Namespace = _DefaultNamespace,
-  TOpt extends TOptions = {},
+  TContext extends string | undefined = undefined,
+  TOpt extends TOptions & { context: TContext } = { context: TContext },
   KPrefix = undefined,
   E = React.HTMLProps<HTMLDivElement>,
->(props: TransProps<Key, Ns, TOpt, KPrefix, E>): React.ReactElement;
+>(props: TransProps<Key, Ns, TOpt, KPrefix, E, TContext>): React.ReactElement;

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.

4 participants