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

Added possibility to use useTranlation returning Array or Object #714

Merged
merged 4 commits into from
Feb 7, 2019
Merged

Added possibility to use useTranlation returning Array or Object #714

merged 4 commits into from
Feb 7, 2019

Conversation

adrai
Copy link
Member

@adrai adrai commented Feb 6, 2019

Added possibility to use useTranlation not only expecting an array but also an object. Should help #712

@coveralls
Copy link

coveralls commented Feb 6, 2019

Coverage Status

Coverage decreased (-0.7%) to 70.556% when pulling cbea832 on adrai:master into 8dc2a10 on i18next:master.

src/index.d.ts Outdated Show resolved Hide resolved
@jamuhl
Copy link
Member

jamuhl commented Feb 7, 2019

@kachkaev opinions about letting users choose? we could provide both options

@kachkaev
Copy link
Contributor

kachkaev commented Feb 7, 2019

What if there is const i18n = useI18n()? as an alternative? It can be used as const { t } = useI18n()iftis bound toi18ncorrectly (no issues withthis`)? The original hook can be marked as deprecated over time if it’s perceived as a less convenient one.

@jamuhl
Copy link
Member

jamuhl commented Feb 7, 2019

@kachkaev i want to avoid another hook. As 95% of the times you like to get some t function based on a given namespace i think this is prefered

const { t } = useTranslation('ns1');
const [t] = useTranslation('ns1');

The only case you like to get the i18n instance is where you need current language or like to access the changeLanguage function. Chances are high you need t function in this component too.

So your approach would be:

const i18n = useI18n();
const { t } = i18n;

while you could just:

const { t, i18n } = useTranslation();

Beside that what we do for HOC and render prop? withI18n?!?

function Extended({ i18n }) {
   const { t } = i18n; 
}

@kachkaev
Copy link
Contributor

kachkaev commented Feb 7, 2019

Oh, I did not realise that JavaScript would allow us to return something that's both an array and an object! In this case, keeping just useTranslation makes perfect sense!

@adrai
Copy link
Member Author

adrai commented Feb 7, 2019

Yes, JavaScript allows to extend arrays.
With this PR you can now choose to do:
const [ t, i18n ] = useTranslation();
or
const { t, i18n } = useTranslation();

@kachkaev
Copy link
Contributor

kachkaev commented Feb 7, 2019

@jamuhl WDYT of updating docs too and encourage users to stick with { t, i18n }? The array can still be still supported for BC, but this will be a 'hidden feature'.

@rosskevin
Copy link
Collaborator

@jamuhl WDYT of updating docs too and encourage users to stick with { t, i18n }? The array can still be still supported for BC, but this will be a 'hidden feature'.

I have added explicit TS tests for both usages, and I agree that the object usage should be promoted as the default (my commit reflects that) especially for js users not using types.

Copy link
Collaborator

@rosskevin rosskevin left a comment

Choose a reason for hiding this comment

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

Very interesting - learned something new about the TS response today! I think object use should be our default.

@jamuhl
Copy link
Member

jamuhl commented Feb 7, 2019

@rosskevin @kachkaev @adrai yes...i will update the docs / samples in favor to using object spread const { t, i18n } = useTranslation();

@jamuhl
Copy link
Member

jamuhl commented Feb 7, 2019

published in react-i18next@10.0.1

@jamuhl
Copy link
Member

jamuhl commented Feb 7, 2019

https://react.i18next.com/latest/usetranslation-hook and all the other places in the docs should reflect this change

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.

5 participants