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

[i18n] Load "en" translations parallely #3102

Merged
merged 19 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b0ab693
load en translations in parallel
rajatvijay Jul 27, 2023
855da7b
Merge branch 'develop' of github.com:centerofci/mathesar into i18n-lo…
rajatvijay Sep 6, 2023
1738aca
renamed loadTranslationsIntoMemory to loadTranslations
rajatvijay Sep 6, 2023
b5c5bde
made translations script async
rajatvijay Sep 6, 2023
9b1c64c
Merge branch 'develop' into i18n-load-en-translations-parallely
rajatvijay Sep 17, 2023
42830ff
move translations path logic to context processor
rajatvijay Sep 18, 2023
8b22c87
use loadAsyncLocale to let vite bundle it as default exported module
rajatvijay Sep 19, 2023
c5ed4bc
lint fixes
rajatvijay Sep 19, 2023
25c89ce
do not read legacy builds for translations files
rajatvijay Sep 19, 2023
43243d3
Merge branch 'develop' into i18n-load-en-translations-parallely
rajatvijay Sep 19, 2023
737effa
render page when loading default lang translations
rajatvijay Sep 19, 2023
a9235ab
Merge branch 'i18n-load-en-translations-parallely' of github.com:cent…
rajatvijay Sep 19, 2023
01a4b4e
Merge branch 'develop' into i18n-load-en-translations-parallely
pavish Sep 20, 2023
6c5a54d
Merge branch 'develop' of github.com:centerofci/mathesar into i18n-lo…
rajatvijay Sep 22, 2023
2d99656
Merge branch 'develop' of github.com:centerofci/mathesar into i18n-lo…
rajatvijay Oct 9, 2023
b11f2b0
move window.Mathesar.translations init in translations file
rajatvijay Oct 9, 2023
e17a308
removed import with explicit extensions
rajatvijay Oct 9, 2023
6c50c83
moved i18n cache to i18n-store file from utils
rajatvijay Oct 9, 2023
e30a065
read new translations format in App.svelte
rajatvijay Oct 9, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion config/context_processors.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,16 @@


def frontend_settings(request):
"""
Hard coding this for now
but will be taken from users model
and cookies later on
"""
preferred_language = 'en'
frontend_settings = {
'development_mode': settings.MATHESAR_MODE == 'DEVELOPMENT',
'manifest_data': get_manifest_data(),
'manifest_data': get_manifest_data(preferred_language),
pavish marked this conversation as resolved.
Show resolved Hide resolved
'preferred_language': preferred_language,
'live_demo_mode': getattr(settings, 'MATHESAR_LIVE_DEMO', False),
'live_demo_username': getattr(settings, 'MATHESAR_LIVE_DEMO_USERNAME', None),
'live_demo_password': getattr(settings, 'MATHESAR_LIVE_DEMO_PASSWORD', None),
Expand Down
32 changes: 27 additions & 5 deletions mathesar/templates/mathesar/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
{% block title %}Home{% endblock %}

{% block styles %}
{% if not development_mode %} {% for css_file in manifest_data.module_css %}
<link rel="stylesheet" href="{% static css_file %}" />
{% endfor %} {% endif %}
{% if not development_mode %}
{% for css_file in manifest_data.module_css %}
<link rel="stylesheet" href="{% static css_file %}" />
{% endfor %}
{% endif %}
{% endblock %}

{% block scripts %}
Expand All @@ -17,6 +19,17 @@
{% endfor %}
{% endif %}

<script type="module">
var isDevelopmentMode = {{ development_mode|yesno:"true,false" }}
var src = isDevelopmentMode ? "{{ client_dev_url }}/src/i18n/{{ preferred_language }}/index.ts" : "{% static manifest_data.module_translations_file %}"
import(src).then(translations => {
window.translations = {
lang: "{{ preferred_language }}",
translationStrings: JSON.stringify(translations.default)
}
})
</script>

{% if development_mode %}
<script type="module" src="{{ client_dev_url }}/@vite/client"></script>
<script type="module" src="{{ client_dev_url }}/src/main.ts"></script>
Expand Down Expand Up @@ -52,11 +65,20 @@
></script>
<script
nomodule
id="vite-legacy-entry"
id="vite-legacy-js-slot"
data-src="{% static manifest_data.legacy_js %}"
>
System.import(
document.getElementById("vite-legacy-entry").getAttribute("data-src")
document.getElementById("vite-legacy-js-slot").getAttribute("data-src")
);
</script>
<script
nomodule
id="vite-legacy-translations-slot"
data-src="{% static manifest_data.legacy_translations_file %}"
>
System.import(
document.getElementById("vite-legacy-translations-slot").getAttribute("data-src")
);
</script>
{% endif %}
Expand Down
15 changes: 14 additions & 1 deletion mathesar/utils/frontend.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from django.core.cache import cache


def get_manifest_data():
def get_manifest_data(preferred_language):
# We don't need the manifest data for local development.
if settings.MATHESAR_MODE == 'DEVELOPMENT':
return {}
Expand All @@ -18,14 +18,27 @@ def get_manifest_data():
with open(settings.MATHESAR_MANIFEST_LOCATION, 'r') as manifest_file:
raw_data = json.loads(manifest_file.read())

translations_file_path = get_translations_file_path(preferred_language)

module_data = raw_data['src/main.ts']
manifest_data['module_css'] = [filename for filename in module_data['css']]
manifest_data['module_js'] = module_data['file']

# Purposefully letting it fail if the translations file for that lang is not in the bundle
manifest_data['module_translations_file'] = raw_data[translations_file_path["module"]]["file"]
pavish marked this conversation as resolved.
Show resolved Hide resolved

legacy_data = raw_data['src/main-legacy.ts']
manifest_data['legacy_polyfill_js'] = raw_data['vite/legacy-polyfills-legacy']['file']
manifest_data['legacy_js'] = legacy_data['file']
manifest_data['legacy_translations_file'] = raw_data[translations_file_path["legacy"]]["file"]

# Cache data for 1 hour
cache.set('manifest_data', manifest_data, 60 * 60)
return manifest_data


pavish marked this conversation as resolved.
Show resolved Hide resolved
def get_translations_file_path(preferred_language):
return {
"module": f'src/i18n/{preferred_language}/index.ts',
"legacy": f'src/i18n/{preferred_language}/index-legacy.ts'
}
29 changes: 18 additions & 11 deletions mathesar_ui/src/App.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,28 @@
import { modal } from './stores/modal';
import { setReleasesStoreInContext } from './stores/releases';
import ModalRecordSelector from './systems/record-selector/ModalRecordSelector.svelte';
import { loadLocaleAsync } from './i18n/i18n-load';
import { setLocale } from './i18n/i18n-svelte';
import type { Locales } from './i18n/i18n-types';
import { loadTranslationsIntoMemory } from './i18n/i18n-load';

/**
* Later the translations file will be loaded
* in parallel to the FE's first chunk
*/
let isTranslationsLoaded = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

from @seancolsen - adding a comment here explaining "why this approach?" since this approach is weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

(() => {
void loadLocaleAsync('en').then(() => {
setLocale('en');
const checkTranslationsInterval = setInterval(() => {
// eslint-disable-next-line prefer-destructuring
const translations:
| { lang: Locales; translationStrings: string }
| undefined =
// @ts-expect-error added by index.html
window.translations;
if (translations) {
loadTranslationsIntoMemory(
translations.lang,
JSON.parse(translations.translationStrings),
);
setLocale(translations.lang);
isTranslationsLoaded = true;
return true;
});
})();
clearInterval(checkTranslationsInterval);
}
}, 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach seems strange to me. If I understand correctly, in index.html we're importing the translation data and sticking it on window. Then here we are looking at window every 100ms to see if it has loaded. This seems really hacky. Why can't we do await import and let vite sort it out?

I wouldn't expect us to need to deal with window. But if we absolutely do, then I'd prefer to use a .d.ts file to provide our own typings for window.translation instead of the @ts-expect-error approach here.

Copy link
Contributor Author

@rajatvijay rajatvijay Sep 6, 2023

Choose a reason for hiding this comment

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

If we do await import vite will download App.svelte first & then start the download for the translations. The code here enables the parallel download for both of the things.

I have added the translations object in global.d.ts 855da7b

Copy link
Member

@pavish pavish Oct 16, 2023

Choose a reason for hiding this comment

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

With the latest set of changes, this approach of checking the data with setInterval is no longer used.


const commonData = preloadCommonData();
if (commonData?.user) {
Expand Down
8 changes: 8 additions & 0 deletions mathesar_ui/src/i18n/i18n-load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,11 @@ export async function loadLocaleAsync(locale: Locales): Promise<void> {
updateTranslationsDictionary(locale, await importLocaleAsync(locale));
loadFormatters(locale);
}

export function loadTranslationsIntoMemory(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this function be named loadTranslations instead? The name loadTranslationsIntoMemory throws me off a bit because it's in the same file as other functions which seem to be also loading things into memory but are not named in a similar manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

locale: Locales,
translations: Translations,
) {
updateTranslationsDictionary(locale, translations);
loadFormatters(locale);
}