Skip to content
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

Implement correct normalization of locales in i18n #8535

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

AMoo-Miki
Copy link
Collaborator

Description

Implement correct normalization of locales in i18n

Also:

  • Stop examining the fragment identifier for possible locale overrides
  • Update relative formats and plural rules for certain locales

Changelog

  • feat: Implement correct normalization of locales in i18n
  • feat: Update relative formats and plural rules for certain locales

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

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 96.29630% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.93%. Comparing base (91fd6d3) to head (f57441c).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
packages/osd-i18n/src/loader.ts 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8535      +/-   ##
==========================================
- Coverage   60.93%   60.93%   -0.01%     
==========================================
  Files        3771     3770       -1     
  Lines       89541    89554      +13     
  Branches    14017    14015       -2     
==========================================
+ Hits        54563    54570       +7     
- Misses      31567    31572       +5     
- Partials     3411     3412       +1     
Flag Coverage Δ
Linux_1 29.04% <68.00%> (+0.03%) ⬆️
Linux_2 56.28% <84.90%> (+<0.01%) ⬆️
Linux_3 37.79% <68.00%> (+0.04%) ⬆️
Linux_4 29.95% <70.37%> (+0.03%) ⬆️
Windows_1 29.06% <68.00%> (+0.03%) ⬆️
Windows_2 56.23% <84.90%> (+<0.01%) ⬆️
Windows_3 37.79% <68.00%> (+0.04%) ⬆️
Windows_4 29.95% <70.37%> (+0.03%) ⬆️

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.

@AMoo-Miki AMoo-Miki force-pushed the fix-i18n-normalization branch from 6f49c5a to 9fc471a Compare October 10, 2024 00:42
Also:
* Stop examining the fragment identifier for possible locale overrides

Signed-off-by: Miki <[email protected]>
@AMoo-Miki AMoo-Miki force-pushed the fix-i18n-normalization branch from 9fc471a to f57441c Compare October 10, 2024 16:34
@virajsanghvi
Copy link
Collaborator

Stop examining the fragment identifier for possible locale overrides

Was this used by anything

export function normalizeLocale(locale: string) {
const { lang, script, region } = localeParser.exec(locale)?.groups || {};
// If parsing failed or the language code was not extracted, return the locale
if (!lang) return locale;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the right thing to accept the bad input or default in this case?

Copy link
Collaborator Author

@AMoo-Miki AMoo-Miki Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guy only does the normalization; (in)validation is handled by comparing it against those that have been registered in a different place.

@@ -100,6 +100,15 @@ export class RenderingService {
opensearchDashboardsConfig as OpenSearchDashboardsConfigType
);

let locale = i18n.getLocale();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this always a defaulted/configured valid locale?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaulted: yes
valid: no

i18n starts with en as the default locale and if configured in the YAML config, it uses that without validating that it has been registered or not; I think this is because while reading the config, the filesystem might not have been read to prime the translation registration cache. At runtime, the locale is validation and if found to not have been registered, en is used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the code, if invalid, then does it potentially lead to a translation file not being loaded? Is this okay because english translations are loaded worst case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the locale is not registered, it would just use English, yes.

@@ -55,7 +55,8 @@ import { getApmConfig } from '../apm';
*/
export function uiRenderMixin(osdServer, server, config) {
const translationsCache = { translations: null, hash: null };
const defaultLocale = i18n.getLocale() || 'en'; // Fallback to 'en' if no default locale is set
// `i18n.getLocale` will always return a value; the 'en' is just to accommodate tests
const defaultLocale = i18n.getLocale() || 'en';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this always be comparable to a normalized locale? (given toLowerCase was dropped)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everyone now goes through normalizeLocale, as opposed to .toLowerCase() which is what the older normalize was doing.

Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this PR removes getLocaleInUrl then introduces a new way to handle locale specification in URLs by checking query parameter const localeOverride = (request.query as any)?.locale?.trim?.();

I think the previous way is to handle both standard query or non-standard one, given the two formats of current osd urls. For the URL format like (https://localhost:9200/app/home#/?locale=cn), where the locale is specified in the hash fragment rather than as a regular query parameter, it seems we can't handle.

Does it mean we will not work on such url and request the standard query format if user need this feature? Should we add some documents somewhere for user?

@AMoo-Miki
Copy link
Collaborator Author

Stop examining the fragment identifier for possible locale overrides

Was this used by anything

No. We haven’t started using any of the theme or locale overrides yet.

@AMoo-Miki
Copy link
Collaborator Author

AMoo-Miki commented Oct 11, 2024

I think the previous way is to handle both standard query or non-standard one, given the two formats of current osd urls.

Does it mean we will not work on such url and request the standard query format if user need this feature?

That is correct. We cannot parse and handle URL fragments on the server-side and as such would rather not entertain them. A similar pattern is used for the theme. These two parameters are exclusively being introduced to temporarily override a setting and are not meant to be suffixed to every URL fragment.

Should we add some documents somewhere for user?

I don't believe these are to be part of our public API. If we decide to make them, we should.

@AMoo-Miki AMoo-Miki merged commit 2b149d2 into opensearch-project:main Oct 11, 2024
67 of 68 checks passed
@opensearch-trigger-bot
Copy link
Contributor

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-8535-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2b149d242cd4f7420c755fe77538b0c56f3677ab
# Push it to GitHub
git push --set-upstream origin backport/backport-8535-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-8535-to-2.x.

AMoo-Miki added a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Oct 11, 2024
…t#8535)

* Implement correct normalization of locales in i18n

Also:
* Stop examining the fragment identifier for possible locale overrides

Signed-off-by: Miki <[email protected]>

* Update relative formats and plural rules for certain locales

Signed-off-by: Miki <[email protected]>

---------

Signed-off-by: Miki <[email protected]>

(cherry picked from commit 2b149d2)
Signed-off-by: Miki <[email protected]>
AMoo-Miki added a commit that referenced this pull request Oct 11, 2024
* Implement correct normalization of locales in i18n

Also:
* Stop examining the fragment identifier for possible locale overrides



* Update relative formats and plural rules for certain locales



---------



(cherry picked from commit 2b149d2)

Signed-off-by: Miki <[email protected]>
Qxisylolo pushed a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Oct 30, 2024
…t#8535)

* Implement correct normalization of locales in i18n

Also:
* Stop examining the fragment identifier for possible locale overrides

Signed-off-by: Miki <[email protected]>

* Update relative formats and plural rules for certain locales

Signed-off-by: Miki <[email protected]>

---------

Signed-off-by: Miki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x distinguished-contributor i18n Internationalization related Issues and PRs v2.18.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants