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.

Sign up for GitHub

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

Jump to bottom

Fix react-i18next to work with TypeScript #261

Merged
merged 2 commits into from
Jun 9, 2017
Merged

Fix react-i18next to work with TypeScript #261

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3017f48
Fix undefined options.react for wait & nsMode
jussikinnula Jun 9, 2017
c52853a
Add intermediate check to set ready() when initialized signal didn't …
jussikinnula Jun 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/translate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

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


this.nsMode = options.nsMode || (this.i18n.options.react && this.i18n.options.react.nsMode) || 'default';
this.nsMode = options.nsMode || (this.i18n.options && this.i18n.options.react && this.i18n.options.react.nsMode) || 'default';

this.state = {
i18nLoadedAt: null,
Expand Down Expand Up @@ -65,6 +65,10 @@ export default function translate(namespaces, options = {}) {
};

this.i18n.on('initialized', initialized);

// 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());
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jussikinnula jussikinnula Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

}
});

Expand Down