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

Allow for Custom DefaultNS typing #1328

Merged

Conversation

JCQuintas
Copy link
Contributor

@JCQuintas JCQuintas commented Jun 4, 2021

Description

This PR allows the declaration of custom defaultNS types and should fix #1302
It also implements a new way of typing the Resources by using the resources key inside CustomTypeOptions instead of the previous API, Resources has been marked as deprecated as well

Usage

// i18n.ts
i18n.use(initReactI18next).init({
  defaultNS: 'ns1'
  ...
});

// react-i18next.d.ts
declare module 'react-i18next' {
  interface CustomTypeOptions {
    defaultNS: 'ns1';
    resources: {
      ns1: typeof ns1;
      ns2: typeof ns2;
    };
  }
}

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

@JCQuintas
Copy link
Contributor Author

I'm not sure how I can test this, but I'm open to suggestions ofc.

Also, should I just create a new MR on https://github.com/i18next/react-i18next-gitbook for documentation or should I wait?

@coveralls
Copy link

coveralls commented Jun 4, 2021

Coverage Status

Coverage remained the same at 96.583% when pulling eba1b18 on JCQuintas:feature/allow-for-custom-namespace-typing into 85889f2 on i18next:master.

@pedrodurek
Copy link
Member

Thanks @JCQuintas, I'll be taking a look and perform some tests this week

ts4.1/index.d.ts Outdated

type ExtractName<T> = T extends { [key in NameKey]: unknown } ? T[NameKey] : TranslationNS;

type DefaultNamespace<T = ExtractName<DefaultNS>> = T extends Fallback<string> ? T : string;
Copy link
Member

Choose a reason for hiding this comment

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

Hey @JCQuintas, my suggestion would be something like this:

export interface CustomTypeOptions {}

type MergeBy<T, K> = Omit<T, keyof K> & K;
type TypeOptions = MergeBy<{
  defaultNS: 'translation';
}, CustomTypeOptions>

type DefaultNamespace<T = TypeOptions['defaultNS']> = T extends Fallback<string> ? T : string;

Then in the future, we could include resources here as well, even other options that may appear. So instead of having one type augmentation for each option, we could have one for all, for example:

interface CustomTypeOptions = {
  resources: typeof resources;
  defaulsNS: 'ns1';
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it. My approach was to not make changes to existing parts of the application cause I don't have a deep understanding of them.

But I believe your approach to accomplish the same and still be flexible to future needs.

I'll make the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it, incorporated the resources key as you suggested with backwards compatibility with the Resources type.

I've added tests to check if this works, though I had to update the typescript version. I don't think this will be an issue since it is a dev dep, but I don't know if it has other side effects I'm unaware of.

Copy link
Member

@pedrodurek pedrodurek Jun 13, 2021

Choose a reason for hiding this comment

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

If we add resources now, we'll have breaking changes. I'm ok with having breaking changes, but we'd need to hold the PR off for a minor or major release.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I've just seen you're keeping Resources and adding it to the default one. Just add a @deprecated decorator to the old Resources interface.

Copy link
Member

Choose a reason for hiding this comment

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

If we add resources now, we'll have breaking changes. I'm ok with having breaking changes, but we'd need to hold the PR off for a minor or major release.

breaking means major

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added @deprecated with a message to use CustomTypeOptions instead. And yes, it should be backwards compatible, so there would be no need for a major release

Choose a reason for hiding this comment

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

@JCQuintas This change did break my team's app. I haven't dug in deep enough to say if it is totally our fault or not - but I can say for sure that:

  • Our app will not build with the latest version
  • I went through about 6 build errors, hacking in some TS fixes, but the build errors just kept coming.
  • I pin the version to "react-i18next": "11.8.6" and the app builds fine

Choose a reason for hiding this comment

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

For anybody who makes it here, we found an easy fix:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ben-denzer do you mind setting up a codesandbox or repository with this issue happening? I can't seem to reproduce this on my tests.

You can use this codesandbox as a starting point
https://codesandbox.io/s/react-i18next-version-1111-type-issue-ce9xo

@JCQuintas
Copy link
Contributor Author

I've also taken the liberty to fix some jest warnings that appeared on the console by excluding the /examples folder

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 🎉 ! Thanks @JCQuintas! Just add the decorator as suggested and we can merge it!

@adrai
Copy link
Member

adrai commented Jun 13, 2021

so merge and minor version?

@JCQuintas
Copy link
Contributor Author

so merge and minor version?

I believe so, yes

@adrai
Copy link
Member

adrai commented Jun 13, 2021

@pedrodurek ok for you?

@pedrodurek
Copy link
Member

@adrai yeah, I'm ok with minor, thanks guys!

@adrai adrai merged commit 626cd8a into i18next:master Jun 14, 2021
@adrai
Copy link
Member

adrai commented Jun 14, 2021

Does something need to be changed here: https://react.i18next.com/latest/typescript ?

v11.11.0 is published

@JCQuintas
Copy link
Contributor Author

Documentation can be changed, should I make a PR for that as well? Any approach I should take?

@adrai
Copy link
Member

adrai commented Jun 14, 2021

Feel free to create a PR

@pedrodurek
Copy link
Member

🙌

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.

Declaring a different defaultNS than "translation" allows unexpected cases on t function. [Typescript]
5 participants