rusackas commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3525685987
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet
configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first
call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
Review Comment:
Good catch, fixed in 9eb535074e... `TranslatorSingleton.ts` already declares
the typed `Window` global, so the casts weren't needed at all.
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet
configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first
call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() =>
{});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
Same fix as the thread above, 9eb535074e.
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet
configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first
call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() =>
{});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
Same fix as the thread above, 9eb535074e.
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet
configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first
call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() =>
{});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() =>
{});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('hello');
+ expect(consoleSpy).toHaveBeenCalledWith(
+ expect.stringMatching(/was called before configure\(\)/),
+ );
+ consoleSpy.mockRestore();
+ });
+});
+
+test('explicit configure() takes precedence over window pack', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
Review Comment:
Same fix as the thread above, 9eb535074e.
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet
configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first
call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() =>
{});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() =>
{});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('hello');
+ expect(consoleSpy).toHaveBeenCalledWith(
+ expect.stringMatching(/was called before configure\(\)/),
+ );
+ consoleSpy.mockRestore();
+ });
+});
+
+test('explicit configure() takes precedence over window pack', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const { configure, t } = require('./TranslatorSingleton');
+ configure({
+ languagePack: {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'es',
+ plural_forms: 'nplurals=2; plural=(n != 1);',
+ },
+ hello: ['hola'],
+ },
+ },
+ },
+ });
+ expect(t('hello')).toBe('hola');
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
Same fix as the thread above, 9eb535074e.
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(),
locale_str))
Review Comment:
mypy infers these locals fine (`dict(...)` and values pulled from a typed
payload), and this file doesn't annotate locals where inference suffices. Also
heads up: #41780 rewrites this block and these locals go away entirely.
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(),
locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
Review Comment:
Same as above... inference covers it, and the local disappears in #41780.
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(),
locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
+ if language and language != "en":
+ # Respect a pack already provided via COMMON_BOOTSTRAP_OVERRIDES_FUNC
+ # (the workaround in #35330 does exactly that), otherwise load the
+ # shared one. `get_language_pack` returns the empty English pack on a
+ # miss, which is the right result (English) when no translation file
+ # exists.
+ pack = payload.get("language_pack") or get_language_pack(language)
Review Comment:
Same as above... inference covers it, and the local disappears in #41780.
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -133,3 +133,73 @@ def test_recaptcha_shown_for_non_federated_auth(
payload = _get_bootstrap()
assert payload["conf"]["RECAPTCHA_PUBLIC_KEY"] == "test-key"
+
+
+# --- language_pack injection --------------------------------------------
+#
+# The Jed pack is injected by `common_bootstrap_payload` (outside the
+# memoized `cached_common_bootstrap_data`) using the shared
+# `superset.translations.utils.get_language_pack`. Tests here cover the
+# wrapper to confirm the pack lands on the payload for non-English
+# locales and is None for English.
+
+
+def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+ app_context: None,
+) -> None:
+ """common.language_pack carries the shared utility's pack for non-en."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
Review Comment:
Test modules are covered by the `tests.*` mypy override
(`disallow_untyped_defs` off), and mypy/ruff pass as-is... skipping to match
the file's convention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
