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

Add support for nsSeparator and defaultNS #739

Merged
merged 5 commits into from
Dec 1, 2021

Conversation

testerez
Copy link
Contributor

@testerez testerez commented Nov 25, 2021

nsSeparator option was required so that I can use natural text as i18n keys. defaultNS was a bonus 馃檪

Option Description Type Default
nsSeparator char to split namespace from key. You should set it to false if you want to use natural text as keys. string | false ':'
defaultNS default namespace used if not passed to useTranslation or in the translation key. string undefined

Also added ns prop in Tans component because I needed a way to set the namespace when nsSeparator=false

Note that those options are available in i18next:

@testerez
Copy link
Contributor Author

Hello @aralroca, could I get a review on this one 馃檹
Thanks!

Copy link
Owner

@aralroca aralroca left a comment

Choose a reason for hiding this comment

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

I see everything fine @testerez. Thanks for your contribution 馃槉

I just have one question: is the nsSeparator required as false? If the Trans component has ns or useTranslation(ns) then it doesn't look at the separator, then when does the nsSeparator=false make sense? Thanks.

@testerez
Copy link
Contributor Author

testerez commented Dec 1, 2021

I just have one question: is the nsSeparator required as false? If the Trans component has ns or useTranslation(ns) then it doesn't look at the separator, then when does the nsSeparator=false make sense? Thanks.

If nsSeparator !== false then you always have the option to add a namespace in the key. It will use the first key found in this order:

  1. i18Key="ns:key"
  2. <Trans ns="ns" />
  3. useTranslation(ns)

nsSeparator=false is required so that the key won't ever be parsed to find a namespace in it.

Does that make sense? Did I get your question right?

@aralroca
Copy link
Owner

aralroca commented Dec 1, 2021

is required so that the key won't ever be parsed to find a namespace in it

I'm still in doubt...

Ideally, the key won't ever be parsed to find a namespace in it when:

  • ns param exists in Trans component: <Trans ns="ns" />
  • ns param exists in useTranslation hook: useTranslation(ns)

That is to say, if the parameter ns exists it would be equivalent to nsSeparator=false and the namespace inside the key would not be looked at.

I understand that this is the current behavior. So I have trouble understanding what nsSeparator=false is doing.

Thanks 馃憦

@testerez
Copy link
Contributor Author

testerez commented Dec 1, 2021

As you can see here, I use in priority the namespace set in the key itself. Which IMO is to be expected. Since the closer to the key is the most specific. I believe this is also how i18next behaves.
Also, as I added the option to have a default namespace, you might want to declare no namespace in Trans or useTranslation. So then how would we know if the key should be parsed or not?

@aralroca
Copy link
Owner

aralroca commented Dec 1, 2021

Okay, now I understand. Thanks for the clarification, now I see it perfectly. Thank you!

@aralroca aralroca merged commit b7b65bc into aralroca:canary Dec 1, 2021
@aralroca
Copy link
Owner

aralroca commented Dec 1, 2021

@all-contributors please add @testerez for code

@allcontributors
Copy link
Contributor

@aralroca

I've put up a pull request to add @testerez! 馃帀

@aralroca
Copy link
Owner

aralroca commented Dec 1, 2021

@testerez I prereleased this in 1.3.0-canary.4!

1 similar comment
@aralroca
Copy link
Owner

aralroca commented Dec 1, 2021

@testerez I prereleased this in 1.3.0-canary.4!

@testerez
Copy link
Contributor Author

testerez commented Dec 1, 2021

Thanks @aralroca! 馃憦

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

Successfully merging this pull request may close these issues.

None yet

2 participants