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.

Sign up for GitHub

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

Jump to bottom

Fix context type checking #2182

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix context type checking #2182

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fa6420d
Fix context typing
May 10, 2024
a080730
Remove unneeded type change
May 10, 2024
9f4f9ca
Merge branch 'i18next:master' into master
stefan-schweiger May 13, 2024
8b225ea
Add tests for context type checking
May 13, 2024
f82a259
improve readability of typing
May 13, 2024
bb47175
add additional test cases
May 13, 2024
11d1472
add additional test cases
May 13, 2024
cbbbe81
Merge branch 'i18next:master' into master
stefan-schweiger May 14, 2024
6787fa0
fix for multiple namespaces
May 14, 2024
1abbcf8
fix typo in test
May 14, 2024
55078b6
Reduce nesting of ContextOfKey
May 14, 2024
c2ba23a
support falsy values
May 15, 2024
1788111
fix error due to formatting
May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 58 additions & 10 deletions test/typescript/custom-types/t.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,16 @@ describe('t', () => {

it('should accept a default context key as a valid `t` function key', () => {
expectTypeOf(t('beverage')).toMatchTypeOf('cold water');

expectTypeOf(t('beverage', { context: undefined })).toMatchTypeOf('cold water');
});

it('should throw error when no `context` is provided using and the context key has no default value ', () => {
// @ts-expect-error dessert has no default value, it needs a context
expectTypeOf(t('dessert')).toMatchTypeOf('error');

// @ts-expect-error dessert has no default value, it needs a context
expectTypeOf(t('dessert', { context: undefined })).toMatchTypeOf('error');
});

it('should work with enum as a context value', () => {
Expand All @@ -174,18 +179,61 @@ describe('t', () => {
expectTypeOf(t('dessert', { context: ctx })).toMatchTypeOf<string>();
});

it('should trow error with string union with missing context value', () => {
it('should throw error with string union with missing context value', () => {
enum DessertMissingValue {
COOKIE = 'cookie',
CAKE = 'cake',
MUFFIN = 'muffin',
ANOTHER = 'another',
}

const ctxMissingValue = DessertMissingValue.ANOTHER;
const getRandomDessert = (): DessertMissingValue =>
Math.random() < 0.5 ? DessertMissingValue.CAKE : DessertMissingValue.ANOTHER;

const ctxRandomValue: DessertMissingValue = getRandomDessert();

// @ts-expect-error Dessert.ANOTHER is not mapped so it must give a type error
expectTypeOf(t('dessert', { context: ctxRandomValue })).toMatchTypeOf<string>();

// @ts-expect-error Dessert.ANOTHER is not mapped so it must give a type error
expectTypeOf(t('dessert', { context: ctxMissingValue })).toMatchTypeOf<string>();
expectTypeOf(t('dessert', { context: DessertMissingValue.ANOTHER })).toMatchTypeOf<string>();

expectTypeOf(
// @ts-expect-error 'another' is not mapped so it must give a type error
t('dessert', { context: 'cake' as 'cake' | 'another' }),
).toEqualTypeOf<'a nice cake'>();

// TODO: edge case which is not correctly detected currently
// expectTypeOf(
// // @ts-expect-error no default context so it must give a type error
// t('dessert', { context: 'cake' as 'cake' | undefined }),
// ).toEqualTypeOf<never>();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcalexiei while using this in my actual app I found this edge case which is uncovered. Wasn't able to figure out how to correctly handle this yet, maybe you have an idea. Overall I think it's not a blocker for the PR as it's still an improvement over the current functionality, but if possible to find a solution for this as well it would be great.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also can in theory be covered by writing code like this value ? t('dessert', { context: value }) : t('dessert'). Which is not ideal but helps.


expectTypeOf(
// @ts-expect-error no default context so it must give a type error
t('dessert', { context: undefined }),
).toEqualTypeOf<unknown>();
});

it('should not throw error with string union with undefined context value if it has a default context', () => {
enum BeverageValue {
BEER = 'beer',
WATER = 'water',
}

const getRandomBeverage = (): BeverageValue | undefined =>
Math.random() < 0.5 ? BeverageValue.BEER : undefined;

const ctxRandomValue = getRandomBeverage();

expectTypeOf(
t('beverage', { context: ctxRandomValue }),
).toMatchTypeOf<'a classic beverage'>();

expectTypeOf(
t('beverage', { context: 'beer' as 'beer' | 'water' | undefined }),
).toEqualTypeOf<'a classic beverage'>();

expectTypeOf(t('beverage', { context: undefined })).toEqualTypeOf<'a classic beverage'>();
});

it('should work with string union as a context value', () => {
Expand All @@ -195,12 +243,12 @@ describe('t', () => {
});

// @see https://github.com/i18next/i18next/issues/2172
// it('should trow error with string union with missing context value', () => {
// expectTypeOf(
// // @ts-expect-error
// t('dessert', { context: 'muffin' as 'muffin' | 'cake' | 'pippo' }),
// ).toMatchTypeOf<string>();
// });
it('should trow error with string union with missing context value', () => {
expectTypeOf(
// @ts-expect-error
t('dessert', { context: 'muffin' as 'muffin' | 'cake' | 'pippo' }),
).toMatchTypeOf<string>();
});
});

it('should work with false plural usage', () => {
Expand Down
22 changes: 21 additions & 1 deletion typescript/t.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,21 @@ export type KeyWithContext<Key, TOpt extends TOptions> = TOpt['context'] extends
? `${Key & string}${_ContextSeparator}${TOpt['context']}`
: Key;

type ContextOfKey<
Key,
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
? Key extends string
? ActualKeys extends `${Key}${_ContextSeparator}${infer Context}`
? Context
: never
: never
: string;

export type TFunctionReturn<
Ns extends Namespace,
Key,
Expand Down Expand Up @@ -264,7 +279,12 @@ export interface TFunction<Ns extends Namespace = DefaultNamespace, KPrefix = un
const ActualOptions extends TOpt & InterpolationMap<Ret> = TOpt & InterpolationMap<Ret>,
>(
...args:
| [key: Key | Key[], options?: ActualOptions]
| [
key: Key | Key[],
options?: Omit<ActualOptions, 'context'> & {
context?: ContextOfKey<Key, Ns, TOpt>;
},
]
| [key: string | string[], options: TOpt & $Dictionary & { defaultValue: string }]
| [key: string | string[], defaultValue: string, options?: TOpt & $Dictionary]
): TFunctionReturnOptionalDetails<Ret, TOpt>;
Expand Down