-
Notifications
You must be signed in to change notification settings - Fork 400
Add build-locales script #442
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
Conversation
7aae6f9
to
612fda3
Compare
bin/build-locales
Outdated
throw e; | ||
} | ||
} | ||
return fileExists; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does eslint get mad if you move the return into the try
/catch
, or do you prefer it this way? It doesn't really matter to me but I'd probably have avoided the extra var (should it be a let
?).
try {
fs.statSync(lockFilePath);
return true;
} catch (e) {
if (e.code !== 'ENOENT') {
throw e;
}
return false;
}
What makes the locking necessary? Won't two processes that are trying to do this at the same time either both work and produce the same output or one would catch up with the other and fail to open a file for writing then bail? |
Yeah you're probably right that the locking is probably not necessary. I based this on the i18n-abide scripts which had the locking. I think I'll remove it and we can always add it back if we have issues. |
0738e05
to
bd23062
Compare
// Set disableSSR to true to debug | ||
// client-side-only render. | ||
if (config.get('disableSSR') === true) { | ||
return Promise.resolve(hydrateOnClient()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ 💥 ✨
return localeToLang(langToLocale(lang)); | ||
} | ||
|
||
export function normalizeLocale(locale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually used, do you anticipate we'll need it?
So after thinking about this being on props some more I went looking in react-router to see if it could help and it looks like it can. The We are creating a On the server we aren't actually creating a function detectLang(params) {
return params.locale || params.query.lang || 'en_US';
}
match({ routes, location: req.url }, (err, redirectLocation, renderProps) => {
const locale = detectLang(renderProps.params);
const jedData = require(`json!../../locale/${locale}/${appInstanceName}.json`);
const i18n = new Jed(jedData);
function createElement(Component, props) {
return <Component {...props} i18n={i18n} />;
}
function renderRouterContext(props) {
return <RouterContext {...props} createElement={createElement} />;
}
return loadOnServer({...renderProps, store})
.then(() => {
const InitialComponent = (
<Provider store={store} key="provider">
<ReduxAsyncConnect {...renderProps} render={renderRouterContext} />
</Provider>
);
});
// ...
return hydrateOnClient({component: InitialComponent});
}); If we took this approach then I believe This might be a big change so no worries if we want to move on but I think it would remove some code. Thoughts? |
I always like the idea of less code :) Even if you pass the i18n down via the router is the router data actually availble all the way down the component tree without intervention? If not it won't buy us much and the current way is explicit in the sense we know what's translated and what's not and we can have an app that isn't localized if we want to. I think it will make sense to land what's working for now and then look at this separately so we can test out the theory. But it's definitely a good call to think of better ways to do the same thing. |
239bae5
to
ede4efc
Compare
Uh oh!
There was an error while loading. Please reload this page.