Code Splitting React Intl Locales #1200
Conversation
| return locale; | ||
| }; | ||
|
|
||
| getBrowserLocale = () => { |
There was a problem hiding this comment.
bind functions in constructor please
| } | ||
| } | ||
|
|
||
| getAvailableLocale = (appLocale) => { |
There was a problem hiding this comment.
bind functions in constructor please
| let detectedLocale; | ||
| if (typeof navigator !== 'undefined') { | ||
| detectedLocale = | ||
| navigator.userLanguage || |
There was a problem hiding this comment.
Lots of logic happening here, you need to split this up and break it down to make it more readable. You should not have multiple ternary operations like this, im surprised eslint didn't pick this up
There was a problem hiding this comment.
I don't think that's too bad here. To me it looks decent, except that (navigator.languages && navigator.languages[0] ? navigator.languages[0] : undefined); could be replaced just with navigator.languages && navigator.languages[0] and it does exactly the same thing.
eslint didn't complain, because there is are no nested ternary operators (which causes eslint errors).
| @@ -73,6 +78,42 @@ export default class Wrapper extends React.PureComponent { | |||
| this.props.getTrendingTopics(); | |||
| } | |||
|
|
|||
| componentWillReceiveProps(nextProps) { | |||
| if (nextProps.locale !== this.props.locale) { | |||
| this.setState({ locale: this.getAvailableLocale(nextProps.locale) }); | |||
There was a problem hiding this comment.
Does this actually work for anyone @Sekhmet @bonustrack ? I took the time and ran this on my local..
and keep seeing these errors when Im switching languages
| componentWillReceiveProps(nextProps) { | ||
| if (nextProps.locale !== this.props.locale) { | ||
| this.setState({ locale: this.getAvailableLocale(nextProps.locale) }); | ||
| import(`react-intl/locale-data/${this.getAvailableLocale(nextProps.locale)}`).then((localeData) => { |
There was a problem hiding this comment.
I know dynamic imports were not recommended in the past before by the creator of redux - facebook/create-react-app#2176
and he works for facebook, so idk about this one and I'm not familiar with these
|
I added some comments, but @bonustrack / @Sekhmet might know more about dynamic importing than me :) |
|
hey @Kennybll, @Sekhmet deployed this.... on here https://busy-master-pr-1200.herokuapp.com/ And it doesn't render anything.... so idk if this will work for us. thanks for the contribution though |
|
It works only locally (running |
|
This PR doesn't seem to be updated, so I created another one: #1237. |
Fixes #1197.
Changes: