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

Not existing context not detected as type error if covered by string union #2172

Open
stefan-schweiger opened this issue Apr 12, 2024 · 8 comments 路 May be fixed by #2182
Open

Not existing context not detected as type error if covered by string union #2172

stefan-schweiger opened this issue Apr 12, 2024 · 8 comments 路 May be fixed by #2182

Comments

@stefan-schweiger
Copy link

stefan-schweiger commented Apr 12, 2024

馃悰 Bug Report

I was told to move this issue here from the i18next/react-i18next#1743 repo.

When you use a string union as the context parameter it's possible to supply values for which a context does not exist and it does not result in an error. If you just use the plain string value or a const it works.

To Reproduce

A minimal reproducible example.

{
  "translation": {
    "testContext1_Test1": "EN: Context1 Test1",
    "testContext1_Test2": "EN: Context1 Test2",
  },
}
const App = () => {
  const { t } = useTranslation('translation');

  return (
    <div>
      <p>
        {t('testContext1', { context: 'Test1' as 'Test1' | 'Test2' | 'Test3' })}
      </p>
      <p>{t('testContext1', { context: 'Test2' })}</p>
      <p>{t('testContext1', { context: 'Test2' as const })}</p>
      <p>
        {t('testContext1', { context: 'Test3' as 'Test1' | 'Test2' | 'Test3' })}
      </p>
      {/* Error: Type '{ context: "Test3"; }' is not assignable to type 'string' */}
      <p>{t('testContext1', { context: 'Test3' })}</p>
      {/* Error: Type '{ context: "Test3"; }' is not assignable to type 'string' */}
      <p>{t('testContext1', { context: 'Test3' as const })}</p>
    </div>
  );
};

Expected behavior

String unions should be correctly detected as errors if they have cases which are not covered by the context.

Your Environment

  • i18next version: 23.11.1
@marcalexiei
Copy link
Member

Honestly I have no idea on how to fix this 馃槩.
If anyone has any idea on how to do this feel free to open a PR.

As a workaround you can use enums. You can find example in this PR: #2173.

@stefan-schweiger
Copy link
Author

@marcalexiei enums actually have the same problem, I've updated my reproduction. But basically enums only work if you assign a known value, but if you assign something which could be any of the enum values it will also happily accept it instead of correctly determining a type error.

@marcalexiei
Copy link
Member

marcalexiei commented Apr 12, 2024

Gotcha, so it's related, if not the same, issue of the string union.
If anyone wants to tackle feel free to open a PR.

@nazarioa
Copy link

nazarioa commented May 2, 2024

I are also seeing this bug... i'll try to fix it.

@stefan-schweiger
Copy link
Author

I'm not even sure if this isn't a feature gap in typescript itself and might warrant to create a issue in the typescript repo. But haven't found the time yet to try to create a minimal reproduction.

@stefan-schweiger
Copy link
Author

stefan-schweiger commented May 10, 2024

Not sure if this maybe breaks something else, but the following code works for my project:

export type ContextOfKey<
  Key extends string,
  Ns extends Namespace = DefaultNamespace,
  TOpt extends TOptions = {},
  Keys extends $Dictionary = KeysByTOptions<TOpt>,
  ActualNS extends Namespace = NsByTOptions<Ns, TOpt>,
  ActualKeys = Keys[$FirstNamespace<ActualNS>],
> = $IsResourcesDefined extends true
  ? ActualKeys extends `${Key}${_ContextSeparator}${infer Context}`
    ? Context
    : never
  : string;

// ...

export interface TFunction<Ns extends Namespace = DefaultNamespace, KPrefix = undefined> {
  $TFunctionBrand: $IsResourcesDefined extends true ? `${$FirstNamespace<Ns>}` : never;
  <
    const Key extends ParseKeys<Ns, TOpt, KPrefix> | TemplateStringsArray,
    const TOpt extends TOptions,
    Ret extends TFunctionReturn<Ns, AppendKeyPrefix<Key, KPrefix>, TOpt>,
    const ActualOptions extends Omit<TOpt, 'context'> &
      InterpolationMap<Ret> & {
        context?: Key extends string ? ContextOfKey<Key, Ns, TOpt> : never;
      } = TOpt &
      InterpolationMap<Ret> & {
        context?: Key extends string ? ContextOfKey<Key, Ns, TOpt> : never;
      },
  >(
    ...args:
      | [key: Key | Key[], options?: ActualOptions]
      | [key: string | string[], options: TOpt & $Dictionary & { defaultValue: string }]
      | [key: string | string[], defaultValue: string, options?: TOpt & $Dictionary]
  ): TFunctionReturnOptionalDetails<Ret, TOpt>;
}
i18next+23.11.3.patch
diff --git a/node_modules/i18next/typescript/t.d.ts b/node_modules/i18next/typescript/t.d.ts
index 6b02eea..bb38ddb 100644
--- a/node_modules/i18next/typescript/t.d.ts
+++ b/node_modules/i18next/typescript/t.d.ts
@@ -205,6 +205,19 @@ export type KeyWithContext<Key, TOpt extends TOptions> = TOpt['context'] extends
   ? `${Key & string}${_ContextSeparator}${TOpt['context']}`
   : Key;

+export type ContextOfKey<
+  Key extends string,
+  Ns extends Namespace = DefaultNamespace,
+  TOpt extends TOptions = {},
+  Keys extends $Dictionary = KeysByTOptions<TOpt>,
+  ActualNS extends Namespace = NsByTOptions<Ns, TOpt>,
+  ActualKeys = Keys[$FirstNamespace<ActualNS>],
+> = $IsResourcesDefined extends true
+  ? ActualKeys extends `${Key}${_ContextSeparator}${infer Context}`
+    ? Context
+    : never
+  : string;
+
 export type TFunctionReturn<
   Ns extends Namespace,
   Key,
@@ -261,7 +274,13 @@ export interface TFunction<Ns extends Namespace = DefaultNamespace, KPrefix = un
     const Key extends ParseKeys<Ns, TOpt, KPrefix> | TemplateStringsArray,
     const TOpt extends TOptions,
     Ret extends TFunctionReturn<Ns, AppendKeyPrefix<Key, KPrefix>, TOpt>,
-    const ActualOptions extends TOpt & InterpolationMap<Ret> = TOpt & InterpolationMap<Ret>,
+    const ActualOptions extends Omit<TOpt, 'context'> &
+      InterpolationMap<Ret> & {
+        context?: Key extends string ? ContextOfKey<Key, Ns, TOpt> : never;
+      } = TOpt &
+      InterpolationMap<Ret> & {
+        context?: Key extends string ? ContextOfKey<Key, Ns, TOpt> : never;
+      },
   >(
     ...args:
       | [key: Key | Key[], options?: ActualOptions]

@marcalexiei can you maybe sanity check this approach if it makes sense?

EDIT: just cloned i18next and ran the typescript tests and seems to be working for those as well.

@marcalexiei
Copy link
Member

marcalexiei commented May 10, 2024

Thanks for spend your time taking a look at this @stefan-schweiger! 馃


Not sure if this maybe breaks something else

Open a PR to see if your patch affects current tested scenarios


can you maybe sanity check this approach if it makes sense?

A PR is a better place to discuss code changes. Another reason to open it 馃槄

FYI right now I don't have to perform a review, maybe in the next days.

@stefan-schweiger stefan-schweiger linked a pull request May 10, 2024 that will close this issue
7 tasks
@stefan-schweiger
Copy link
Author

@marcalexiei created the PR let me know when you find the time to take a look.

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