Change the locale dynamically by adding &i18n-locale to URL #7686
Conversation
The main issue was the inability to dynamically change the locale in OpenSearch Dashboards. Currently we need to update config file and i18nrc.json. This PR allows users to switch to a different locale (e.g., from English to Chinese) by appending or modifying the 'i18n-locale' parameter in the URL. * getAndUpdateLocaleInUrl: If a non-default locale is found, this function reconstructs the URL with the locale parameter in the correct position. * updated the ScopedHistory class, allowing it to detect locale changes and trigger reloads as necessary. * modify the i18nMixin, which sets up the i18n system during server startup, to register all available translation files during server startup, not just the current locale. * update the uiRenderMixin to accept requests for any registered locale and dynamically load and cache translations for requested locales. Signed-off-by: Anan Zhuang <[email protected]>
ananzh
requested review from
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi,
mengweieric,
LDrago27,
virajsanghvi,
sejli and
joshuali925
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7686 +/- ##
==========================================
+ Coverage 63.69% 63.79% +0.09%
==========================================
Files 3633 3640 +7
Lines 80135 80519 +384
Branches 12699 12813 +114
==========================================
+ Hits 51043 51366 +323
- Misses 25984 26017 +33
- Partials 3108 3136 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/core/public/locale_helper.ts
Outdated
| ]; | ||
|
|
||
| for (const pattern of patterns) { | ||
| const match = url.match(pattern); |
There was a problem hiding this comment.
can this do something like
instead of regex?
There was a problem hiding this comment.
Oh I remember why I switch to regex. The searchParams property of the URL object is designed to work with standard query parameters, which are typically preceded by a question mark (?) in the URL.
For the URL like http://localhost:5601/app/home/&i18n-locale-zh-CN, it doesn't follow the standard format for query parameters.The & is being used as if it's starting a query string, but without a preceding ?.The searchParams approach would not work directly and searchParams.get('i18n-locale') will return null. I think this is the same issue as our later discussion.
If we always have standard query parameters then we are good to switch to searchParams. But if in the future we want to translate urls like our current home page http://localhost:5603/xqy/app/home#/ or management http://localhost:5603/xqy/app/management then we can't use this. I am trying to make this feature applicable for future extension. This approach allows the app which needs this feature to simply attach &i18n-locale at the end without caring about the url. So I think the ultimate decision is aligned with the answers of #7686 (comment). I can update accordingly but I still think we should also validate the url to avoid any crush.
@kavilla @ashwin-pc what do you guys think?
There was a problem hiding this comment.
I think it's answered in #7686 (comment)
When this feature exposes to users we should advertise it as "add a URL parameter of i18n-locale to change locale", not "append
&i18n-localeto change locale", so i don't think we need to handle invalid URL parameters
| // from '/translations/en.json' to '/translations/zh-CN.json' | ||
| injectedMetadata.i18n.translationsUrl = injectedMetadata.i18n.translationsUrl.replace( | ||
| /\/([^/]+)\.json$/, | ||
| `/${urlLocale}.json` |
There was a problem hiding this comment.
if user sets an invalid locale through URL, is it the same behavior as setting it through config/opensearch_dashboards.yml and OSD UI won't break?
There was a problem hiding this comment.
It will break just as you set an invalid one in config/opensearch_dashboards.yml. Both will use load in i18n
export async function load(translationsUrl: string) {
// Once this package is integrated into core OpenSearch Dashboards we should switch to an abstraction
// around `fetch` provided by the platform, e.g. `kfetch`.
const response = await fetch(translationsUrl, {
credentials: 'same-origin',
});
if (response.status >= 300) {
throw new Error(`Translations request failed with status code: ${response.status}`);
}
init(await response.json());
}
this fetch will call locale route in src/legacy/ui/ui_render/ui_render_mixin.js
server.route({
path: '/translations/{locale}.json',
method: 'GET',
config: { auth: false },
handler: async (request, h) => {
const { locale } = request.params;
const normalizedLocale = locale.toLowerCase();
const registeredLocales = i18nLoader.getRegisteredLocales().map((l) => l.toLowerCase());
// Function to get or create cached translations
const getCachedTranslations = async (localeKey, getTranslationsFn) => {
if (!translationsCache[localeKey]) {
const translations = await getTranslationsFn();
translationsCache[localeKey] = {
translations: JSON.stringify(translations),
hash: createHash('sha1').update(JSON.stringify(translations)).digest('hex'),
};
}
return translationsCache[localeKey];
};
let cachedTranslations;
if (normalizedLocale === defaultLocale.toLowerCase()) {
// Default locale
cachedTranslations = await getCachedTranslations(defaultLocale, () =>
i18n.getTranslation()
);
} else if (registeredLocales.includes(normalizedLocale)) {
// Other registered locales
cachedTranslations = await getCachedTranslations(normalizedLocale, () =>
i18nLoader.getTranslationsByLocale(locale)
);
} else {
// Locale not found
throw Boom.notFound(`Unknown locale: ${locale}`);
}
return h
.response(cachedTranslations.translations)
.header('cache-control', 'must-revalidate')
.header('content-type', 'application/json')
.etag(cachedTranslations.hash);
},
});
for config change it will go to clause normalizedLocale === defaultLocale.toLowerCase() and for dynamic it will go to else if (registeredLocales.includes(normalizedLocale)) because defaultLocale is the current i18n's locale. So if we have a dynamic locale, we will check if it has a translation file otherwise will throw throw Boom.notFound(Unknown locale: ${locale});, which is same as if you pass an invalid locale to config.
There was a problem hiding this comment.
sorry. my comment is not correct. I think there is a bug in the previous behavior: once we defined in the config this i18n.getLocale() !== locale.toLowerCase() is always true so we never call throw Boom.notFound(Unknown locale: ${locale});
handler(request, h) {
// OpenSearch Dashboards server loads translations only for a single locale
// that is specified in `i18n.locale` config value.
const { locale } = request.params;
if (i18n.getLocale() !== locale.toLowerCase()) {
throw Boom.notFound(`Unknown locale: ${locale}`);
}
// Stringifying thousands of labels and calculating hash on the resulting
// string can be expensive so it makes sense to do it once and cache.
if (translationsCache.translations == null) {
translationsCache.translations = JSON.stringify(i18n.getTranslation());
translationsCache.hash = createHash('sha1')
.update(translationsCache.translations)
.digest('hex');
}
return h
.response(translationsCache.translations)
.header('cache-control', 'must-revalidate')
.header('content-type', 'application/json')
.etag(translationsCache.hash);
},
I am not sure if this a proper behavior because user has no idea that they pass the wrong locale. If we want to stick to this behavior we could change to
else {
// Locale not found
cachedTranslations = await getCachedTranslations('en', () =>
i18nLoader.getTranslationsByLocale('en')
);
}
src/core/public/locale_helper.ts
Outdated
| * 2. Removes the locale parameter from its original position. | ||
| * 3. Reconstructs the URL, placing the locale parameter in the correct position | ||
| * based on the URL structure to ensure proper parsing by OpenSearch Dashboards. | ||
| * 4. Updates the browser's URL without causing a page reload. |
There was a problem hiding this comment.
skimmed through seems the purpose is to handle invalid URL param syntax where & should be used instead of ?. Do we need to handle that? i'm not sure if anything else in OSD does similar handling, since usually it's user's responsibility to correctly use URL parameters
There was a problem hiding this comment.
From my tests, I found
http://localhost:5601/app/home#/&i18n-locale=zh-CNwill not workhttp://localhost:5601/app/home#/? query= (...) &i18n-locale=zh-CNcan work.
The error will cause a blank page without proper prompt error and hard to trace. I think /&i18n-locale=zh-CN might be interpreted as a path rather than a query parameter, leading to routing errors or the parameter being ignored.
There was a problem hiding this comment.
http://localhost:5601/app/home#/&i18n-locale=zh-CN this is not correct syntax, anything other parameters like http://localhost:5601/app/home#/&xxx also doesn't work. My point is there are many different support url params, do we need to do this special handling just for i18n-locale?
There was a problem hiding this comment.
the reason i'm asking this is that if user puts something like /app/home#/&i18n-locale=en&embed=true or the correct way /app/home#/?i18n-locale=en&embed=true i don't think this logic handles it, the implementation might not be generic enough to handle URL params
and i don't think it's OSD's responsibility to autocorrect invalid URL parameters?
There was a problem hiding this comment.
Thank you for this feedback. I think I might miss understood the request on some details. I'd like to clarify a few points:
- I assumed that
&i18n-localewould always be attached at the end of the URL. Is this assumption correct in all cases? - Regarding format and validation, what specific aspects should we be concerned about? Are there any edge cases we need to handle?
- For cases like
http://localhost:5601/app/home#/&i18n-locale=zh-CN, should our function handle this, or should the app be responsible for ensuring the correct URL structure (ex http://localhost:5601/app/home/&i18n-locale=zh-CN) before our function is called?
- If the app is responsible for URL structure, how should we document this requirement for developers using this feature?
There was a problem hiding this comment.
- I assumed that &i18n-locale would always be attached at the end of the URL. Is this assumption correct in all cases?
I think we just want to give a reasonable API for client to override OSD locale, which URL parameter should be enough in this case. When this feature exposes to users we should advertise it as "adding a URL parameter of i18n-locale to change locale", not "append &i18n-locale to change locale", so i don't think we need to handle invalid URL parameters
- Regarding format and validation, what specific aspects should we be concerned about? Are there any edge cases we need to handle?
Since there are similar existing implementation in OSD (embed parameter), I think following the same implementation should be enough
for 3 and 4 i'm not sure what "app" refers to, could you clarify? But user/consumer of the feature should be responsible to provide correct URL parameter, the function should read it in a way respecting to searchParams standard
| `/${urlLocale}.json` | ||
| ); | ||
| } else { | ||
| i18n.setLocale('en'); |
There was a problem hiding this comment.
I remember this is for user removing the i18n-locale part from url after first add it.
|
Nice! The feature looks good. A few questions and comments on the implementation though:
|
|
|
sorry haven't dived into it deep and it probably was covered somewhere but how come we didn't want to base it on the locale from the user browser? |
ananzh
added
v2.17.0
i18n
|
Made some adjustments according to the comments
2024-08-14_09-05-25.mp4 |
@kgcreative any concerns with this toast pattern? |
|
|
||
| if (data.warning) { | ||
| // Store the warning to be displayed after core system setup | ||
| (window as any).__i18nWarning = data.warning; |
There was a problem hiding this comment.
is it common to store this in window?
There was a problem hiding this comment.
I think it is not uncommon, especially in situations where we need to pass information from sever side to browser that doesn't have a direct communication channel.
Signed-off-by: Anan Zhuang <[email protected]>
|
Seems consistent with the way we handle toasts across the application today. I would prefer we use the |
Hi @kgcreative since the feature is for developer only, toasts might not be necessary, cuz user will never managing the url. Therefore I just change to console warning. 
|
Signed-off-by: Anan Zhuang <[email protected]>
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7686-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 21b8a2fa772dfd3b7ca718c5d95877f2c9699421
# Push it to GitHub
git push --set-upstream origin backport/backport-7686-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.xThen, create a pull request where the |
…cale to URL (opensearch-project#7686) Backport PR: opensearch-project#7686 * Change the locale dynamically by adding &i18n-locale to URL The main issue was the inability to dynamically change the locale in OpenSearch Dashboards. Currently we need to update config file and i18nrc.json. This PR allows users to switch to a different locale (e.g., from English to Chinese) by appending or modifying the 'i18n-locale' parameter in the URL. * getAndUpdateLocaleInUrl: If a non-default locale is found, this function reconstructs the URL with the locale parameter in the correct position. * updated the ScopedHistory class, allowing it to detect locale changes and trigger reloads as necessary. * modify the i18nMixin, which sets up the i18n system during server startup, to register all available translation files during server startup, not just the current locale. * update the uiRenderMixin to accept requests for any registered locale and dynamically load and cache translations for requested locales. --------- Signed-off-by: Anan Zhuang <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
…cale to URL (#7686) (#7781) Backport PR: #7686 * Change the locale dynamically by adding &i18n-locale to URL The main issue was the inability to dynamically change the locale in OpenSearch Dashboards. Currently we need to update config file and i18nrc.json. This PR allows users to switch to a different locale (e.g., from English to Chinese) by appending or modifying the 'i18n-locale' parameter in the URL. * getAndUpdateLocaleInUrl: If a non-default locale is found, this function reconstructs the URL with the locale parameter in the correct position. * updated the ScopedHistory class, allowing it to detect locale changes and trigger reloads as necessary. * modify the i18nMixin, which sets up the i18n system during server startup, to register all available translation files during server startup, not just the current locale. * update the uiRenderMixin to accept requests for any registered locale and dynamically load and cache translations for requested locales. --------- Signed-off-by: Anan Zhuang <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
The main issue was the inability to dynamically change the locale in OpenSearch Dashboards. Currently we need to update config file and i18nrc.json.
This PR allows users to switch to a different locale (e.g., from English to Chinese) by appending or modifying the 'i18n-locale' parameter in the URL.
Issues Resolved
NA
Screenshot
no-query.mp4
complex.mov
Can be shared for both embed or link snapshot but not saved object. This is expected, bc this locale is attached to the url not saved in saved obj.
share-link.mov
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration