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

feat(types): add returnNull and returnEptyString options to TypeOptions interface #1341

Merged

Conversation

ImADrafter
Copy link
Contributor

Working with react-i18next and typescript, I found that returnNull and returnEptyString options were not represented in typescript signatures.

By providing keys for these values inside the CustomTypeOptions, we can refine the return types for TFunction.

const dictionary = {
    ...
    nullValue: null,
    emptyString: "",
}

t('nullValue') // By default, signature returns null.
t('emptyString') // By default, signature returns ""
declare module 'react-i18next' {
    interface CustomTypeOptions extends MyCustomTypeOptions {
        returnNull: false,
        returnEmptyString: false,
    }
}

t('nullValue') // After moduleAugmentation, signature returns string, representing the key.
t('emptyString') // After moduleAugmentation, signature returns string, representing the key.

Notice that these keys must be aligned with the actual options in the i18next instance in order to represent the actual behaviour of the library on runtime.

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • documentation is changed or added

@coveralls
Copy link

coveralls commented Jul 13, 2021

Coverage Status

Coverage increased (+0.2%) to 96.583% when pulling 228deb7 on ImADrafter:feat-return-null-and-return-empty-string into 7cfab97 on i18next:master.

ts4.1/index.d.ts Outdated
Comment on lines 123 to 139
type NonNullableTranslation<
T,
U extends Partial<CustomTypeParameters>
> = U['returnNull'] extends false ? (T extends null ? string : T) : T;

type NonEmptyStringTranslation<
T,
U extends Partial<CustomTypeParameters>
> = U['returnEmptyString'] extends false ? (T extends '' ? string : T) : T;

/**
* Checks if user has enabled `returnEmptyString` and `returnNull` options to retrieve correct values.
*/
export type NormalizeByTypeOptions<
T,
U extends Partial<CustomTypeParameters> = TypeOptions
> = NonNullableTranslation<NonEmptyStringTranslation<T, U>, U>;
Copy link
Member

Choose a reason for hiding this comment

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

As both helpers are pretty much the same, you could make it more reusable, something like:

Suggested change
type NonNullableTranslation<
T,
U extends Partial<CustomTypeParameters>
> = U['returnNull'] extends false ? (T extends null ? string : T) : T;
type NonEmptyStringTranslation<
T,
U extends Partial<CustomTypeParameters>
> = U['returnEmptyString'] extends false ? (T extends '' ? string : T) : T;
/**
* Checks if user has enabled `returnEmptyString` and `returnNull` options to retrieve correct values.
*/
export type NormalizeByTypeOptions<
T,
U extends Partial<CustomTypeParameters> = TypeOptions
> = NonNullableTranslation<NonEmptyStringTranslation<T, U>, U>;
type TypeOptionsFallback<T, U, R> = U extends false ? (T extends R ? string : T) : T
/**
* Checks if user has enabled `returnEmptyString` and `returnNull` options to retrieve correct values.
*/
export type NormalizeByTypeOptions<
T,
U extends Partial<CustomTypeParameters> = TypeOptions,
R = TypeOptionsFallback<T, U['returnEmptyString'], ''>
> = TypeOptionsFallback<R, U['returnNull'], null>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! I applied your suggestion, but changing the generic parameters naming. What do you think ? @pedrodurek

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

ts4.1/index.d.ts Outdated
Comment on lines 118 to 121
type CustomTypeParameters = {
returnNull: boolean;
returnEmptyString: boolean;
};
Copy link
Member

Choose a reason for hiding this comment

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

Can't we simply rely on type CustomTypeParameters = Pick<TypeOptions, 'returnNull' | 'returnEmptyString'>;?

In case we can't, we can make both parameters optional instead of using partial in three different places:

type CustomTypeParameters = {
  returnNull?: boolean;
  returnEmptyString?: boolean;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't make that Pick, because the types for returnNull and returnEmptyString would be true, not boolean :(

Applied your suggestion about making the keys optional and remove that Partial @pedrodurek

const emptyStringValue2: ReturnEmptyString = '';

// @ts-expect-error: '"non-empty-string"' is not assignable to type '""'
const nonEmptyStringValue2: ReturnEmptyString = 'non-empty-string';
Copy link
Member

Choose a reason for hiding this comment

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

I'm always in favour of testing everything integrated by testing both flags in the useTranslation, but I'm afraid it's not possible to toggle them on/off 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I don't know if there's a way to make the augmentation twice to check both flags. If you have any idea, just let me know :) @pedrodurek

…ns interface

- Add utility types in order to return correct types for both returnEmtpyString and returnNull options
- Add test cases for both options
@ImADrafter ImADrafter force-pushed the feat-return-null-and-return-empty-string branch from d01a300 to 228deb7 Compare July 22, 2021 09:56
@adrai
Copy link
Member

adrai commented Jul 25, 2021

is this ready to merge?

Copy link
Member

@pedrodurek pedrodurek left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@jamuhl jamuhl merged commit 91bacea into i18next:master Jul 26, 2021
@jamuhl
Copy link
Member

jamuhl commented Jul 26, 2021

published in react-i18next@11.11.4

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

Successfully merging this pull request may close these issues.

None yet

5 participants