-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Webpack config in dev and prod include copywebpack for static transla… #1491
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
…tions.json Change in i18n to serve the translations from a different path
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.
Almost there, but the way that the server exposes the copied translations to the client needs to be updated.
@@ -7,7 +7,7 @@ const fallbackLng = ['en-US']; | |||
const availableLanguages = ['en-US', 'es-419']; | |||
|
|||
const options = { | |||
loadPath: '/translations/{{lng}}/translations.json', |
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.
I think this should stay as '/translations/{{lng}}/translations.json'
, but the static file config in server/server.js
needs to change...
server/server.js
Outdated
@@ -75,11 +75,11 @@ app.use(corsMiddleware); | |||
app.options('*', corsMiddleware); | |||
|
|||
// Body parser, cookie parser, sessions, serve public assets | |||
|
|||
app.use('/translations', Express.static('translations/locales/')); |
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.
The path needs to point to the location of the copied translations...
app.use('/translations', Express.static(path.resolve(__dirname, '../dist/static/translations'));
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.
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.
@catarak, you're right that L79 will serve /dist/static/translations
but it will sent with a max-age of 1 day. Everything bundled by WebPack uses the file's content hash in the filename (e.g. app.c41b13c86f9f5577f9cc.js
). The browser can cache this for a long time (in principle, forever) since it'll never change. If the bundle's updated, it has a new filename and the browser fetches the file at the new URL.
For now, we're not versioning the translations assets using a content hash in the filename so we should serve them from /dist/static/translations
but with a different cache strategy. Using an E-Tag with no max-age
means that the browser can make a quick conditional GET request to check if the file's been updated. If it hasn't been updated, no data is received but if it has, it will be sent the latest file.
In future we should probably use the same content-hash-in-filename strategy for these files too, but it's a bit more complex so this is a fix in the short-term that gets staging working again.
@oruburos this line (L78) needs to be:
app.use(
'/locales',
Express.static(
path.resolve(__dirname, '../dist/static/locales'),
{
// Browsers must revalidate for changes to the locale files
// It doesn't actually mean "don't cache this file"
// See: https://jakearchibald.com/2016/caching-best-practices/
setHeaders: res => res.setHeader('Cache-Control', 'no-cache')
}
)
);
…tions.json Change in i18n to serve the translations from a different path
…tions.json Change in i18n to serve the translations from a different path
This looks good to me! Thanks for working on this ✨ |
This PR copies the translations in dist/static as suggested in
#1478 (comment)
Fixes #1478
I have verified that this pull request:
npm run lint
)Fixes #1478