[i18n] Load "en" translations parallely #3102

rajatvijay · 2023-07-27T04:36:36Z

Relates #2927

Technical details

This PR does the following things -

Hard code the language as en in the backend
Load translations file in a script tag in index.html
Injects translations in the window global
Show the loader till the translations file is downloaded.

Loom
https://www.loom.com/share/9689a91822094772851277d61bd376c8

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the develop branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no
visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

seancolsen

I have some questions about this code for @rajatvijay.

@pavish I'd like you review this as well since you understand this area of the app much better than me.

Also, I haven't tried running this in prod mode. I've been holding off on doing that while reviewing these PRs in hopes that someone would do it once all the i18n changes are in. But this work definitely needs some testing in prod mode by someone at some point. I'm not not sure when the best time to do that is, given how much is still in flux with the i18n work.

seancolsen · 2023-08-09T22:24:15Z

mathesar_ui/src/i18n/i18n-load.ts

@@ -30,3 +30,11 @@ export async function loadLocaleAsync(locale: Locales): Promise<void> {
  updateTranslationsDictionary(locale, await importLocaleAsync(locale));
  loadFormatters(locale);
 }
+
+export function loadTranslationsIntoMemory(


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.

seancolsen · 2023-08-09T22:31:03Z

mathesar_ui/src/App.svelte

+  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);


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.

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

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

pavish · 2023-08-31T16:49:05Z

@rajatvijay Could you please resolve the conflicts in this PR?

…ad-en-translations-parallely

pavish

@rajatvijay I've added my review comments.

I'm not sure if you've tested the PR thoroughly before raising it. The changes do not work. Please ensure that you've tested any new changes in both development and production modes.

mathesar/utils/frontend.py

mathesar_ui/src/App.svelte

mathesar/templates/mathesar/index.html

config/context_processors.py

rajatvijay · 2023-09-11T13:12:45Z

mathesar_ui/src/App.svelte

-      };
+  import { loadTranslations } from './i18n/i18n-load';
+
+  let isTranslationsLoaded = false;


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

pavish

@rajatvijay and I had a review call and these were the changes that were discussed.

Move the window.translations initialization into the translation ts files.
Remove promise from index.html and change it to <script type="module" src={src}>.
Add legacy support back.
Don't stringify translation strings.
Use "Mathesar" instead of "mathesar".
Remove '.js' extension in all i18n import statements.

(Optional):

Namespace translation files if possible.
Move objects that are only used to store translations from 'i18n-util' into a separate file.
Use a single point of access to set and retrieve locale store values.

…ad-en-translations-parallely

pavish

@rajatvijay Looks good to me, approved! Thanks for addressing all my concerns.

@seancolsen Based on your comment here, I assume you might be interested in taking a look at this PR. I'm holding off merging for you and @rajatvijay to sort it out.

load en translations in parallel

b0ab693

rajatvijay requested a review from seancolsen July 27, 2023 04:36

rajatvijay assigned seancolsen Jul 27, 2023

rajatvijay added the pr-status: review A PR awaiting review label Jul 27, 2023

rajatvijay added this to the v0.1.3 milestone Jul 27, 2023

rajatvijay marked this pull request as ready for review July 27, 2023 04:37

rajatvijay mentioned this pull request Jul 28, 2023

[i18n] Install typesafe-i18n & translates one component #3099

Merged

7 tasks

Base automatically changed from i18n-django-templates-translatable to develop August 9, 2023 19:21

seancolsen reviewed Aug 9, 2023

View reviewed changes

seancolsen assigned pavish and rajatvijay and unassigned seancolsen Aug 9, 2023

rajatvijay modified the milestones: v0.1.3, v0.1.4 Aug 17, 2023

rajatvijay added 2 commits September 6, 2023 15:14

Merge branch 'develop' of github.com:centerofci/mathesar into i18n-lo…

855da7b

…ad-en-translations-parallely

renamed loadTranslationsIntoMemory to loadTranslations

1738aca

rajatvijay requested a review from pavish September 6, 2023 09:54

made translations script async

b5c5bde

rajatvijay removed their assignment Sep 6, 2023

pavish requested changes Sep 7, 2023

View reviewed changes

pavish assigned rajatvijay and unassigned pavish Sep 7, 2023

pavish added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Sep 7, 2023

seancolsen mentioned this pull request Sep 8, 2023

[i18n] Save preferred_language for auth user in db #3103

Merged

7 tasks

rajatvijay commented Sep 11, 2023

View reviewed changes

rajatvijay added 3 commits September 18, 2023 00:23

Merge branch 'develop' into i18n-load-en-translations-parallely

9b1c64c

move translations path logic to context processor

42830ff

use loadAsyncLocale to let vite bundle it as default exported module

8b22c87

Merge branch 'develop' into i18n-load-en-translations-parallely

01a4b4e

pavish requested changes Sep 21, 2023

View reviewed changes

pavish assigned rajatvijay and unassigned pavish Sep 21, 2023

pavish added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Sep 21, 2023

Merge branch 'develop' of github.com:centerofci/mathesar into i18n-lo…

6c5a54d

…ad-en-translations-parallely

seancolsen modified the milestones: v0.1.4, Next release Oct 2, 2023

rajatvijay added 5 commits October 9, 2023 16:01

Merge branch 'develop' of github.com:centerofci/mathesar into i18n-lo…

2d99656

…ad-en-translations-parallely

move window.Mathesar.translations init in translations file

b11f2b0

removed import with explicit extensions

e17a308

moved i18n cache to i18n-store file from utils

6c50c83

read new translations format in App.svelte

e30a065

rajatvijay requested a review from pavish October 9, 2023 12:03

rajatvijay assigned pavish and unassigned rajatvijay Oct 9, 2023

rajatvijay added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Oct 9, 2023

pavish approved these changes Oct 16, 2023

View reviewed changes

pavish assigned seancolsen and rajatvijay and unassigned pavish Oct 16, 2023

pavish added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Oct 16, 2023

seancolsen approved these changes Oct 25, 2023

View reviewed changes

seancolsen added this pull request to the merge queue Oct 25, 2023

Merged via the queue into develop with commit da201e0 Oct 25, 2023
38 of 41 checks passed

seancolsen deleted the i18n-load-en-translations-parallely branch October 25, 2023 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[i18n] Load "en" translations parallely #3102

[i18n] Load "en" translations parallely #3102

rajatvijay commented Jul 27, 2023 •

edited

seancolsen left a comment

seancolsen Aug 9, 2023

rajatvijay Sep 6, 2023

seancolsen Aug 9, 2023

rajatvijay Sep 6, 2023 •

edited

pavish Oct 16, 2023 •

edited

pavish commented Aug 31, 2023

pavish left a comment

rajatvijay Sep 11, 2023 •

edited

rajatvijay Sep 19, 2023

pavish left a comment •

edited

pavish left a comment

[i18n] Load "en" translations parallely #3102

[i18n] Load "en" translations parallely #3102

Conversation

rajatvijay commented Jul 27, 2023 • edited

Checklist

Developer Certificate of Origin

seancolsen left a comment

Choose a reason for hiding this comment

seancolsen Aug 9, 2023

Choose a reason for hiding this comment

rajatvijay Sep 6, 2023

Choose a reason for hiding this comment

seancolsen Aug 9, 2023

Choose a reason for hiding this comment

rajatvijay Sep 6, 2023 • edited

Choose a reason for hiding this comment

pavish Oct 16, 2023 • edited

Choose a reason for hiding this comment

pavish commented Aug 31, 2023

pavish left a comment

Choose a reason for hiding this comment

rajatvijay Sep 11, 2023 • edited

Choose a reason for hiding this comment

rajatvijay Sep 19, 2023

Choose a reason for hiding this comment

pavish left a comment • edited

Choose a reason for hiding this comment

pavish left a comment

Choose a reason for hiding this comment

rajatvijay commented Jul 27, 2023 •

edited

rajatvijay Sep 6, 2023 •

edited

pavish Oct 16, 2023 •

edited

rajatvijay Sep 11, 2023 •

edited

pavish left a comment •

edited