Angular - Hybrid localization support#24731
Conversation
There was a problem hiding this comment.
Pull request overview
Adds hybrid UI localization support to ABP Angular by allowing client-side JSON localization resources to be loaded per language and merged with backend-provided resources (with UI taking precedence).
Changes:
- Introduces
UILocalizationServiceto load/assets/localization/{culture}.json(configurable) and register resources intoLocalizationService. - Updates
LocalizationServicemerge order so UI localizations override backend texts. - Exports and initializes the new service conditionally via core options.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| npm/ng-packs/packages/core/src/lib/services/ui-localization.service.ts | New service that loads UI localization JSON per culture and registers/records loaded resources. |
| npm/ng-packs/packages/core/src/lib/services/localization.service.ts | Adjusts merge order so UI resources override backend resources. |
| npm/ng-packs/packages/core/src/lib/services/index.ts | Exports the new UI localization service from the services barrel. |
| npm/ng-packs/packages/core/src/lib/providers/core-module-config.provider.ts | Conditionally initializes UILocalizationService during app init based on options. |
| npm/ng-packs/packages/core/src/lib/models/common.ts | Adds uiLocalization options to core ABP root configuration model. |
| const remoteTexts = entry[1]; | ||
| let resource = local?.get(resourceName) || {}; | ||
| resource = { ...resource, ...remoteTexts }; | ||
| // UI > Backend priority: UI localization'lar backend'i override eder |
There was a problem hiding this comment.
The inline comment is partly in Turkish ("UI localization'lar backend'i override eder"), which is inconsistent with the rest of the codebase and makes the intent harder to understand for non-Turkish readers. Please rewrite it in English (and keep it consistent with the comment above about UI > Backend priority).
| // UI > Backend priority: UI localization'lar backend'i override eder | |
| // UI > Backend priority: UI localizations override backend localizations |
| export interface LocalizationResource { | ||
| [resourceName: string]: Record<string, string>; | ||
| } |
There was a problem hiding this comment.
The exported interface name LocalizationResource is easy to confuse with ABP.LocalizationResource (already defined in models/common.ts) but represents a different shape (JSON file content keyed by resourceName). Consider renaming it to something more specific (e.g., UiLocalizationFile, UiLocalizationDictionary, etc.) to avoid ambiguity for consumers importing from the services barrel.
| distinctUntilChanged(), | ||
| switchMap(culture => this.loadLocalizationFile(culture)), | ||
| shareReplay(1), | ||
| ) | ||
| .subscribe(); |
There was a problem hiding this comment.
shareReplay(1) is applied to a stream that is immediately subscribed to and not exposed to other consumers, so it provides no benefit here and can be misleading. Consider removing it, or alternatively expose a shared observable if caching/reuse is intended.
| // Initialize UILocalizationService if UI-only mode is enabled | ||
| const options = inject(CORE_OPTIONS); | ||
| if (options?.uiLocalization?.enabled) { | ||
| inject(UILocalizationService); | ||
| } |
There was a problem hiding this comment.
The comment says "UI-only mode" but this feature appears to be hybrid UI+backend localization merging (based on uiLocalization options and the merge logic in LocalizationService). Please adjust the comment to accurately describe what is being enabled/initialized here.
| private loadLocalizationFile(culture: string) { | ||
| const config = this.options.uiLocalization; | ||
| if (!config?.enabled) return of(null); | ||
|
|
||
| const basePath = config.basePath || '/assets/localization'; | ||
| const url = `${basePath}/${culture}.json`; | ||
|
|
||
| return this.http.get<LocalizationResource>(url).pipe( |
There was a problem hiding this comment.
This PR introduces new behavior (loading /assets/localization/{culture}.json and merging it with backend resources, including error handling when the file is missing), but there are no tests covering UILocalizationService. Since the package already has service-level tests under src/lib/tests (e.g. localization.service.spec.ts), please add tests that verify loading on language changes and UI > backend priority merging.
| .pipe( | ||
| distinctUntilChanged(), | ||
| switchMap(culture => this.loadLocalizationFile(culture)), | ||
| shareReplay(1), |
There was a problem hiding this comment.
Hello @erdemcaygor thank you for developing a side feature to enhance the localization. I agree with the co-pilot command here.
The shareReplay(1) usage here is not needed since there is exactly one subscription for this observable.
| currentLanguage$ = this.sessionState.getLanguage$(); | ||
|
|
||
| ngOnInit() { | ||
| // Yüklenen UI localization'ları göster |
There was a problem hiding this comment.
you can remove Turkish Comment Line
| import { SessionStateService } from './session-state.service'; | ||
| import { CORE_OPTIONS } from '../tokens/options.token'; | ||
|
|
||
| export interface UILocalizationResource { |
There was a problem hiding this comment.
The interface name UILocalizationResource might be confused with ABP.LocalizationResource (defined in abp\npm\ng-packs\packages\core\src\lib\models\common.ts), as they represent different structures:
ABP.LocalizationResource:{ resourceName: string, texts: Record<string, string> }UILocalizationResource:{ [resourceName: string]: Record<string, string> }(dictionary format)
Consider renaming to something more descriptive like UiLocalizationDictionary or UiLocalizationFileContent to better reflect its purpose as the JSON file content format.
Resolves #24675