-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Use permanent cache for translation files on production #181377
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
Changes from all commits
823baaf
207825c
b86d1e5
a5f5355
f66e10f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,54 +6,81 @@ | |
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import { createHash } from 'crypto'; | ||
| import { i18n } from '@kbn/i18n'; | ||
| import { schema } from '@kbn/config-schema'; | ||
| import type { IRouter } from '@kbn/core-http-server'; | ||
|
|
||
| const MINUTE = 60; | ||
| const HOUR = 60 * MINUTE; | ||
| const DAY = 24 * HOUR; | ||
|
|
||
| interface TranslationCache { | ||
| translations: string; | ||
| hash: string; | ||
| } | ||
|
|
||
| export const registerTranslationsRoute = (router: IRouter, locale: string) => { | ||
| export const registerTranslationsRoute = ({ | ||
| router, | ||
| locale, | ||
| translationHash, | ||
| isDist, | ||
| }: { | ||
| router: IRouter; | ||
| locale: string; | ||
| translationHash: string; | ||
| isDist: boolean; | ||
| }) => { | ||
| let translationCache: TranslationCache; | ||
|
|
||
| router.get( | ||
| { | ||
| path: '/translations/{locale}.json', | ||
| validate: { | ||
| params: schema.object({ | ||
| locale: schema.string(), | ||
| }), | ||
| }, | ||
| options: { | ||
| access: 'public', | ||
| authRequired: false, | ||
| }, | ||
| }, | ||
| (ctx, req, res) => { | ||
| if (req.params.locale.toLowerCase() !== locale.toLowerCase()) { | ||
| return res.notFound({ | ||
| body: `Unknown locale: ${req.params.locale}`, | ||
| }); | ||
| } | ||
| if (!translationCache) { | ||
| const translations = JSON.stringify(i18n.getTranslation()); | ||
| const hash = createHash('sha1').update(translations).digest('hex'); | ||
| translationCache = { | ||
| translations, | ||
| hash, | ||
| }; | ||
| } | ||
| return res.ok({ | ||
| headers: { | ||
| 'content-type': 'application/json', | ||
| 'cache-control': 'must-revalidate', | ||
| etag: translationCache.hash, | ||
| ['/translations/{locale}.json', `/translations/${translationHash}/{locale}.json`].forEach( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept the old route for two reasons:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This path is not a public API so i dont think we should worry about custom integrations especially since i dont see any use case for this.. Maybe worth creating an issue to remove it later on in a separate PR |
||
| (routePath) => { | ||
| router.get( | ||
| { | ||
| path: routePath, | ||
| validate: { | ||
| params: schema.object({ | ||
| locale: schema.string(), | ||
| }), | ||
| }, | ||
| options: { | ||
| access: 'public', | ||
| authRequired: false, | ||
| }, | ||
| }, | ||
| body: translationCache.translations, | ||
| }); | ||
| (ctx, req, res) => { | ||
| if (req.params.locale.toLowerCase() !== locale.toLowerCase()) { | ||
| return res.notFound({ | ||
| body: `Unknown locale: ${req.params.locale}`, | ||
| }); | ||
| } | ||
| if (!translationCache) { | ||
| const translations = JSON.stringify(i18n.getTranslation()); | ||
| translationCache = { | ||
| translations, | ||
| hash: translationHash, | ||
| }; | ||
| } | ||
|
|
||
| let headers: Record<string, string>; | ||
| if (isDist) { | ||
| headers = { | ||
| 'content-type': 'application/json', | ||
| 'cache-control': `public, max-age=${365 * DAY}, immutable`, | ||
| }; | ||
| } else { | ||
| headers = { | ||
| 'content-type': 'application/json', | ||
| 'cache-control': 'must-revalidate', | ||
| etag: translationCache.hash, | ||
| }; | ||
|
Comment on lines
+65
to
+75
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept the |
||
| } | ||
|
|
||
| return res.ok({ | ||
| headers, | ||
| body: translationCache.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.
I may have missed it, but is it worth having a jest integration style test for this "isDist" Boolean and how it affects response headers?