Change the locale dynamically by adding &i18n-locale to URL #7686

ananzh · 2024-08-12T17:22:55Z

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.

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.

Issues Resolved

NA

Screenshot

For url without query params

no-query.mp4

For url with query params

complex.mov

For unsupport locale, it will break with proper hint with a 404 not found error. This is different from current behavior which will use en without any hints.

Share link
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

feat: Change the locale dynamically by adding &i18n-locale to URL

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

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 <ananzh@amazon.com>

codecov · 2024-08-12T17:40:35Z

Codecov Report

Attention: Patch coverage is 89.70588% with 7 lines in your changes missing coverage. Please review.

Project coverage is 63.79%. Comparing base (1bf63e3) to head (cea33d4).
Report is 14 commits behind head on main.

Files	Patch %	Lines
src/core/public/locale_helper.ts	82.35%	3 Missing ⚠️
src/legacy/server/i18n/index.ts	0.00%	3 Missing ⚠️
src/core/public/osd_bootstrap.ts	93.33%	0 Missing and 1 partial ⚠️

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

Flag	Coverage Δ
Linux_1	`29.82% <0.00%> (-0.77%)`	⬇️
Linux_2	`55.84% <60.29%> (+0.21%)`	⬆️
Linux_3	`40.35% <0.00%> (?)`
Linux_4	`31.29% <41.66%> (-0.02%)`	⬇️
Windows_1	`29.84% <0.00%> (-0.77%)`	⬇️
Windows_2	`55.80% <60.29%> (+0.21%)`	⬆️
Windows_3	`40.35% <0.00%> (-0.21%)`	⬇️
Windows_4	`31.29% <41.66%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joshuali925

thanks, just some questions

joshuali925 · 2024-08-12T17:30:03Z

src/core/public/locale_helper.ts

+  ];
+
+  for (const pattern of patterns) {
+    const match = url.match(pattern);


can this do something like

OpenSearch-Dashboards/src/core/public/chrome/chrome_service.tsx

Line 141 in 75d67c0

const isEmbedded = new URL(location.hash.slice(1), location.origin).searchParams.has('embed');

instead of regex?

will try this one.

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?

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-locale to change locale", so i don't think we need to handle invalid URL parameters

joshuali925 · 2024-08-12T17:32:10Z

src/core/public/osd_bootstrap.ts

+    // from '/translations/en.json' to '/translations/zh-CN.json'
+    injectedMetadata.i18n.translationsUrl = injectedMetadata.i18n.translationsUrl.replace(
+      /\/([^/]+)\.json$/,
+      `/${urlLocale}.json`


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?

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.

got it, thanks

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') ); }

joshuali925 · 2024-08-12T17:37:24Z

src/core/public/locale_helper.ts

+ * 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.


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

From my tests, I found

http://localhost:5601/app/home#/&i18n-locale=zh-CN will not work

http://localhost:5601/app/home#/? query= (...) &i18n-locale=zh-CN can 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.

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?

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?

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-locale would 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?

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

joshuali925 · 2024-08-12T17:42:59Z

src/core/public/osd_bootstrap.ts

+      `/${urlLocale}.json`
+    );
+  } else {
+    i18n.setLocale('en');


why is this needed?

I remember this is for user removing the i18n-locale part from url after first add it.

ashwin-pc · 2024-08-13T07:19:47Z

Nice! The feature looks good. A few questions and comments on the implementation though:

Will the locale always be there in the URL? if so can we make it optional?
Why did we change the behaviour when the locale is missing? Cant we load the default locale and show the error in the console instead? Why break the page for the user?
Why are you supporting invalid URL formats? Whats the need for this?

ananzh · 2024-08-13T17:14:23Z

Nice! The feature looks good. A few questions and comments on the implementation though:

Will the locale always be there in the URL? if so can we make it optional?

Why did we change the behaviour when the locale is missing? Cant we load the default locale and show the error in the console instead? Why break the page for the user?

Why are you supporting invalid URL formats? Whats the need for this?

For Q1:
locale can be removed. if no specified locale, we will use the default one specified in the config opensearch_dashboards.yml or en if not specified.
For Q2:
I could change the current hehaviour to not break the page and add a prompt to hint user.
For Q3:
We don't have to support invalid URL formats. Should we just let break the page if the url is not valid?

kavilla · 2024-08-13T19:48:33Z

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 · 2024-08-14T16:15:12Z

Made some adjustments according to the comments

no re-construction on invalid or non-standard url: only toast warning for invalid or non-standard url
no break if locale is not supported (no translation file)

2024-08-14_09-05-25.mp4

ashwin-pc · 2024-08-14T17:06:43Z

Made some adjustments according to the comments

no re-construction on invalid or non-standard url: only toast warning for invalid or non-standard url

no break if locale is not supported (no translation file)

2024-08-14_09-05-25.mp4

@kgcreative any concerns with this toast pattern?

joshuali925 · 2024-08-14T17:09:53Z

packages/osd-i18n/src/core/i18n.ts

+
+  if (data.warning) {
+    // Store the warning to be displayed after core system setup
+    (window as any).__i18nWarning = data.warning;


is it common to store this in window?

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 <ananzh@amazon.com>

kgcreative · 2024-08-14T20:01:00Z

Seems consistent with the way we handle toasts across the application today. I would prefer we use the globe, questionInCircle, or alert icons instead of the help icon.

ananzh · 2024-08-15T00:56:58Z

Seems consistent with the way we handle toasts across the application today. I would prefer we use the globe, questionInCircle, or alert icons instead of the help icon.

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 <ananzh@amazon.com>

joshuali925

lgtm, thanks

opensearch-trigger-bot · 2024-08-15T20:07:53Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7686-to-2.x.

…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 <ananzh@amazon.com> 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 <ananzh@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

github-actions bot added the distinguished-contributor label Aug 12, 2024

opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Aug 12, 2024

Changeset file for PR opensearch-project#7686 created/updated

75d67c0

joshuali925 reviewed Aug 12, 2024

View reviewed changes

ananzh assigned joshuali925 Aug 14, 2024

ananzh added v2.17.0 i18n Internationalization related Issues and PRs backport 2.x labels Aug 14, 2024

opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Aug 14, 2024

Changeset file for PR opensearch-project#7686 created/updated

18875d7

joshuali925 reviewed Aug 14, 2024

View reviewed changes

fix comments 2

2052a66

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

ananzh force-pushed the dynamic-i18n branch from 18875d7 to 2052a66 Compare August 14, 2024 17:43

ananzh force-pushed the dynamic-i18n branch from e5451c2 to c01f3d7 Compare August 15, 2024 02:16

opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Aug 15, 2024

Changeset file for PR opensearch-project#7686 created/updated

298de3d

ananzh force-pushed the dynamic-i18n branch from 298de3d to bafe4ba Compare August 15, 2024 02:39

opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Aug 15, 2024

Changeset file for PR opensearch-project#7686 created/updated

659d9c4

fix tests

77eff19

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

ananzh force-pushed the dynamic-i18n branch from 659d9c4 to 77eff19 Compare August 15, 2024 05:50

Changeset file for PR opensearch-project#7686 created/updated

cea33d4

joshuali925 approved these changes Aug 15, 2024

View reviewed changes

LDrago27 approved these changes Aug 15, 2024

View reviewed changes

ashwin-pc merged commit 21b8a2f into opensearch-project:main Aug 15, 2024
67 checks passed

opensearch-trigger-bot bot added the failed backport label Aug 15, 2024

ananzh mentioned this pull request Aug 21, 2024

[2.x manual backport]Change the locale dynamically by adding &i18n-locale to URL (#7686) #7781

Merged

7 tasks

AMoo-Miki removed the failed backport label Aug 22, 2024

This was referenced Sep 7, 2024

[FEATURE] It seems that there is currently only i18n support for English? opensearch-project/dashboards-i18n#32

Open

[BUG] Docker Image's I18N_LOCALE environment variable does not seem to work #8075

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the locale dynamically by adding &i18n-locale to URL #7686

Change the locale dynamically by adding &i18n-locale to URL #7686

ananzh commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

joshuali925 left a comment

joshuali925 Aug 12, 2024

ananzh Aug 12, 2024

ananzh Aug 13, 2024 •

edited

Loading

joshuali925 Aug 13, 2024

joshuali925 Aug 12, 2024

ananzh Aug 12, 2024

joshuali925 Aug 12, 2024

ananzh Aug 13, 2024

joshuali925 Aug 12, 2024

ananzh Aug 12, 2024 •

edited

Loading

joshuali925 Aug 12, 2024

joshuali925 Aug 12, 2024 •

edited

Loading

ananzh Aug 13, 2024 •

edited

Loading

joshuali925 Aug 13, 2024

joshuali925 Aug 12, 2024

ananzh Aug 12, 2024

ashwin-pc commented Aug 13, 2024

ananzh commented Aug 13, 2024 •

edited

Loading

kavilla commented Aug 13, 2024

ananzh commented Aug 14, 2024

ashwin-pc commented Aug 14, 2024

joshuali925 Aug 14, 2024

ananzh Aug 14, 2024

kgcreative commented Aug 14, 2024

ananzh commented Aug 15, 2024 •

edited

Loading

joshuali925 left a comment

opensearch-trigger-bot bot commented Aug 15, 2024

Change the locale dynamically by adding &i18n-locale to URL #7686

Change the locale dynamically by adding &i18n-locale to URL #7686

Conversation

ananzh commented Aug 12, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Aug 12, 2024 • edited Loading

Codecov Report

joshuali925 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananzh Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananzh Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuali925 Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

ananzh Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc commented Aug 13, 2024

ananzh commented Aug 13, 2024 • edited Loading

kavilla commented Aug 13, 2024

ananzh commented Aug 14, 2024

ashwin-pc commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgcreative commented Aug 14, 2024

ananzh commented Aug 15, 2024 • edited Loading

joshuali925 left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 15, 2024

ananzh commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

ananzh Aug 13, 2024 •

edited

Loading

ananzh Aug 12, 2024 •

edited

Loading

joshuali925 Aug 12, 2024 •

edited

Loading

ananzh Aug 13, 2024 •

edited

Loading

ananzh commented Aug 13, 2024 •

edited

Loading

ananzh commented Aug 15, 2024 •

edited

Loading