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

Fix react-i18next to work with TypeScript #261

Merged
merged 2 commits into from Jun 9, 2017
Merged

Conversation

@jussikinnula
Copy link
Contributor

jussikinnula commented Jun 9, 2017

There were two minor problems for react-i18next to work with TypeScript. First of all options and options.react might be undefined (on initialization). Then also it might be that if wait is set "true" in props, that there's indefinite wait if i18n doesn't get "initialized" signal through.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 9, 2017

Coverage Status

Coverage increased (+0.05%) to 94.068% when pulling c52853a on jussikinnula:master into 5aeabfe on i18next:master.


// In case of race condition, that 'initialized' never comes - do immediately
// check ready state + if i18n is initialized.
setTimeout(() => !this.state.ready && this.i18n.isInitialized && ready());

This comment has been minimized.

Copy link
@jamuhl

jamuhl Jun 9, 2017

Member

Why it is not sufficient to have the ready() call in the first if block if (this.i18n.isInitialized) .... How can that be not set in the if - but than in the else block? How did you get into that condition...never got that myself.

This comment has been minimized.

Copy link
@jussikinnula

jussikinnula Jun 9, 2017

Author Contributor

If this.i18n.isInitialized is false, and we don't get signal when i18n was initialized - the application will have to wait indefinitely.

Our problem was that if setting wait "true" in the application container - the initialization never finished. With wait set to "false" it did load, which lead on fixing the issue.

To overcome that race condition we could alternatively set the setTimeout() before this.i18n.isInitialized if block. Then we could call ready() immediately. But I would prefer a minimal change, probably at some point the core logic could be made a bit cleaner.

This comment has been minimized.

Copy link
@jamuhl

jamuhl Jun 9, 2017

Member

if isInitialized is false - you bind to onInitialized event in the else block -> isInitialized is still false and will be set to true and the event will be called. so ready() will be called.

Point is i don't see how that code should not work unless you do some multithread magic.

So basically you say:

If this.i18n.isInitialized is false, and we don't get signal when i18n was initialized

How can it be you do not get the initialized event? Really would love to understand that:

  1. translate hoc: not initialized
  2. translate hoc: binds initialized event
  3. i18next finishes init: sets isInitialized and emits the event
  4. translate hoc: calls ready

This comment has been minimized.

Copy link
@jamuhl

jamuhl Jun 9, 2017

Member

As you also got i18next.options not set -> it really seems you do not call i18next.init before rendering to dom.

This comment has been minimized.

Copy link
@jussikinnula

jussikinnula Jun 9, 2017

Author Contributor

The root cause to the issue must be something, that when I'm having the translate() instantiated to the application container - the i18next initialization takes just a fraction of a second longer than mounting the translate. I did some further checking and it seems that there's three setTimeout()'s in i18next with zero time set as parameter, which actually would explain the potential for race condition.

Just to let you know our client is structured like this, so you can see i18next init is the first which gets called:

import { render, unmountComponentAtNode } from 'react-dom';
import { createStore } from 'redux';
import { Provider } from 'react-redux';
import { I18nextProvider, translate } from 'react-i18next';

import i18n from './i18n';
import configureStore from './store/configureStore';
import './styles/main.styl';

...

And i18n is:

import * as LngDetector from 'i18next-browser-languagedetector';
import translations from './translations';
import languages from './languages';

i18n
  .use(LngDetector)
  .init({
    whitelist: languages,
    fallbackLng: 'en',
    debug: true,
    resources: {
      ...translations
    },
    ns: ['common'],
    defaultNS: 'common',
    interpolation: {
      escapeValue: false, // not needed for react!!
    },
  });

export default i18n;

And the profiling (put some console log to the else block likeif (this.i18n.isInitialized) { ... } else { console.log("XXX"); ... }) did output the order, so i18next initialization did happen after the "XXX" printed out.

This comment has been minimized.

Copy link
@jamuhl

jamuhl Jun 9, 2017

Member

init finished after so ended in the else block...where the listener was bound for initialized event already. And https://github.com/i18next/i18next/blob/master/src/i18next.js#L121 was calling that.

a) it is initialized
or
b) it was bound to listen to initialized

single thread - there can nothing happen between a) or b) so i have no idea where a c) should come from where initialized was emitted before. As set initialized and emitting is also the same tick.

Could you please provide a sample to reproduce? Really would love to get this solved...

@@ -19,9 +19,9 @@ export default function translate(namespaces, options = {}) {
namespaces = namespaces || this.i18n.options.defaultNS;
if (typeof namespaces === 'string') namespaces = [namespaces];

if (!wait && this.i18n.options.wait || (this.i18n.options.react && this.i18n.options.react.wait)) wait = this.i18n.options.wait || this.i18n.options.react.wait;
if (!wait && this.i18n.options && (this.i18n.options.wait || (this.i18n.options.react && this.i18n.options.react.wait))) wait = true;

This comment has been minimized.

Copy link
@jamuhl

jamuhl Jun 9, 2017

Member

How does it come i18next.options are not yet set...do you call i18next.init after rendering?

This comment has been minimized.

Copy link
@jussikinnula

jussikinnula Jun 9, 2017

Author Contributor

The actual problem is that if this.i18n.options.react is empty, since it's not even set in the @types/i18next typings. Similarly this.i18n.options.wait is not defined in the typings. To overcome TypeScript compiler of stopping the compilation, we need to do check for the upmost common identifier - which on this case is this.i18n.options.

This comment has been minimized.

Copy link
@jamuhl

jamuhl Jun 9, 2017

Member

Ok. Can live with that...although i would prefer someone updates the typescript definitions (as i personally am a absolute non-fan of typescript - cause of reasons seeing here - having to cheat in code to let a compiler not stop building...what a world).

This comment has been minimized.

Copy link
@jussikinnula

jussikinnula Jun 9, 2017

Author Contributor

I see your point, but I'm only doing my part on making it so that people who have selected TypeScript can live with it :-) ...

@jamuhl jamuhl merged commit 245a03d into i18next:master Jun 9, 2017
2 of 3 checks passed
2 of 3 checks passed
codeclimate 1 new issue (2 fixed)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 94.068%
Details
@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Jun 9, 2017

published in react-i18next@4.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.