ryandrewjohnson / react-localize-redux

Dead simple localization for your React components
https://ryandrewjohnson.github.io/react-localize-redux-docs/
MIT License
374 stars 88 forks source link

error handling for null active language. #45

Closed da1nerd closed 6 years ago

da1nerd commented 6 years ago

In https://github.com/ryandrewjohnson/react-localize-redux/blob/master/src/Localize.js#L17 where state is mapped to props you are retrieving the active language and immediately accessing the code property.

I'm encountering a race condition in which the locale has not finished initializing before Localize.js maps it's state. This is because I'm loading about 30 translation files and doing some extra processing to support falling back to the closest matching locale. Therefore, the active language is undefined and the maptStateToProps throws an error Cannot read property 'code' of undefined.

Example:

// Parent component
class Main extends React.Component {
  componentWillMount() {
    this.props.loadLocalization() // this is a thunk
  }
  render() {
    return <Child/> // thunk above has not finished by the time this renders.
  }
}

// child component
class Child extends React.Component {

}
Child = localize(Child, 'locale')

Solution

I resolved this by connecting getActiveLangauge on the parent class and rendering a loading screen while the active language was null. However, I think the documentation/logging for this could be clearer.

One solution would be to simply provide some placeholder values in https://github.com/ryandrewjohnson/react-localize-redux/blob/master/src/Localize.js#L17 if the active language was null. This probably isn't the best solution because it encourages developers to be lazy.

The other option would be to intentionally throw an error with a message indicating that they probably didn't finish initializing the locale. Then add a small section to the documentation regarding these asynchronous situations.

ryandrewjohnson commented 6 years ago

I can't really know for sure without seeing more of your project, but the very first action creator you should be dispatching is initialize before you do anything else. In most applications you would be calling this as soon as your app boots up. For example I usually make this call on my Root components componentWillMount lifecycle method.

There really should be no case where a race condition occurs, as your active language will be set synchronously as soon as initialize is dispatched. The only thing I can think of is if you're attempting to wrap your app's root component with localize. In which case your component would be attempting to access parts of the state that have yet to be initialized.

In short make sure that you've dispatched initialize before rendering any components that depend on localize or any connected components that rely on react-localize-redux selectors.

As a side note I will update the Getting Started section in the docs to make this more clear.

da1nerd commented 6 years ago

In our case I'm calling initialize from within a thunk with promises which loads the translations asynchronously. Therefore introducing the race.

e.g.

const myInit = () => {
  return (dispatch) => {
    return fs.readir(somedir).then((files) => {
     ...
     dispatch(initialize(...));
    });
  }
}
da1nerd commented 6 years ago

Another note regarding all of this. My current fix of waiting until getActiveLangauge returns a non-null value does not completely work because it's possible for translate to be called before the first translation has been added since initializing and adding a language are two different actions.

Therefore the best solution in my case seems to simply create my own custom action that indicates the locale loading is finished. I am now uncertain whether it is reasonable for your library to handle asynchronous situations at all.

If developers are going so far as to load the locale asynchronously (as in a thunk with promises) it's not too much more work for them to also add an action to indicate loading is complete. I just think adding some documentation regarding this caveat would be very helpful.

BTW. This is a great library. Thanks!

ryandrewjohnson commented 6 years ago

Thanks for the kudos - i'm glad you like the library.

Going back to this example you provided. I'm still not sure why you are waiting to call initialize until after your files are loaded. If files in this case is your translation data you don't actually need this data for the initialize action. If you can tell me what's in the files data, and what in that data initialize is dependent on I might better understand.

da1nerd commented 6 years ago

files is a list of all my translations e.g. English-en_US.json, German-de_DE.json, etc. From those file names I parse a list of languages that will be passed to initialize. Later I read each file and pass it to addTranslationForLanguage.

ryandrewjohnson commented 6 years ago

Thanks for the update - I understand your workflow now.

I have pushed a fix that prevents the uncaught error Cannot read property 'code' of undefined from being thrown if a user happens to Localize a component before they've dispatched initialize.

Fixed in #52

da1nerd commented 6 years ago

awesome thanks!