[Translator] Add "by-domain" translation dumper #1927

maelanleborgne · 2024-06-19T12:58:12Z

Q	A
Bug fix?	no
New feature?	yes
Issues	Fix #1225
License	MIT

Will follow #1930

Adds a new translation dumper to dump translations by domains in separated files. Theses files can be imported and registered with the new registerDomain(domain) js function, and accessed using the existing trans function by passing the translation key as a first argument.

TODO:

Add documentation
Add entry to changelog
Add better TS typing to the new functions/object
Add a TS declaration file for the new domains files ?

smnandre · 2024-06-19T13:49:10Z

src/Translator/src/DependencyInjection/UxTranslatorExtension.php

+        $webpackDumperDefinition = $container->getDefinition('ux.translator.translations_dumper.webpack');
+        $webpackDumperDefinition->setArgument(0, $config['dump_directory']);
+
+        $assetMapperDumperDefinition = $container->getDefinition('ux.translator.translations_dumper.asset_mapper');
+        // Setting to the default value because the folder isn't configurable with asset mapper
+        $assetMapperDumperDefinition->setArgument(0, $this->processConfiguration($configuration, [])['dump_directory']);


I think we should use some configuration for the user to chose which service he wants to use..
... and alias it as "TranslationDumper".

smnandre · 2024-06-19T13:52:03Z

src/Translator/src/FileDumper/AssetMapperTranslationsFileDumper.php

+            $domainSplit[$domainFileName] ??= [];
+            $domainSplit[$domainFileName][$translationId] ??= ['id' => $translationId, 'translations' => []];
+            $domainSplit[$domainFileName][$translationId]['translations'][$domain] ??= [];
+            $domainSplit[$domainFileName][$translationId]['translations'][$domain][$locale] = $translationDetails['message'];


So you'd keep all the locale in one file ?

Yes that's the plan right now, otherwise we'd have to import too many modules (1 per combination of domain/locale). Though a filter on the dumped locales could help remove the unnecessary ones. Unless you have something else in mind ?

smnandre · 2024-06-19T13:54:52Z

src/Translator/src/FileDumper/AssetMapperTranslationsFileDumper.php

+                if (\in_array($domain, $this->excludedDomains, true)) {
+                    continue;
+                }
+                if (0 !== \count($this->domains) && !\in_array($domain, $this->domains, true)) {
+                    continue;
+                }


I think we maybe should use "includedDomain" / "excludedDomain" or something similar, maybe just me but the difference in naming makes it less clear

smnandre · 2024-06-19T13:58:59Z

src/Translator/src/FileDumper/TranslationConfigDumper.php

+        $this->filesystem->dumpFile($this->dumpDir.'/configuration.js', sprintf(
+            "export const localeFallbacks = %s;\n",
+            json_encode($this->getLocaleFallbacks(...$catalogues), \JSON_THROW_ON_ERROR)
+        ));


Maybe a simple concatenation here ?

smnandre · 2024-06-19T13:59:57Z

src/Translator/src/FileDumper/TranslationConfigDumper.php

+        $this->filesystem->dumpFile($this->dumpDir.'/configuration.d.ts', <<<'TS'
+import { LocaleType } from '@symfony/ux-translator';
+
+export declare const localeFallbacks: Record<LocaleType, LocaleType>;
+TS
+        );


Is this file static ?

smnandre · 2024-06-19T14:02:53Z

src/Translator/src/FileDumper/WebpackTranslationsDumper.php

+    }
+
+    public function setExcludedDomains(array $domains = []): void
+    {
+        $this->excludedDomains = $domains;
+    }
+
+    public function setDomains(array $domains): void
+    {
+        $this->domains = $domains;
    }


Wondering if this should either

be in an abstractDumper

be in a new TranslationDumpProvider (find a better name^^)

Because there is a certain amount of duplicated code there

smnandre · 2024-06-19T14:04:19Z

src/Translator/tests/FileDumper/AssetMapperTranslationsDumperTest.php

+        "translations": {
+            "foobar": {


is this "domain as key" necessary ?

smnandre · 2024-06-19T14:06:16Z

src/Translator/assets/dist/translator_controller.js

+function resetRegisteredDomains() {
+    _registeredTranslations = {};
+}


What is the use case ?

It was mainly to reset the translations between tests, but I changed the behavior to allow overriding already registered translations, so I got rid of this method.

smnandre · 2024-06-19T14:14:01Z

src/Translator/src/FileDumper/WebpackTranslationsDumper.php

@@ -30,8 +30,11 @@
 * @phpstan-type Locale string
 * @phpstan-type MessageId string
 */
-class TranslationsDumper


This would be a massive BC break... not sure the "experimental" would be enough :)

Maybe should we keep it as alias / deprecated ?

@kbond ?

What about keeping the TranslationDumper as a wrapper that would call either or both the Webpack and AssetMapper dumper + the config dumper ?

Why not ! That could reduce some code duplication too!

Kocal

Hi, and thanks for your contribution! :)

I didn't look for everything, but isn't there too much things for a single PR?
I see dump per domains, global domains filters, but also a new dumper for AssetMapper... is it too much?

maelanleborgne · 2024-06-19T15:52:30Z

Hi, and thanks for your contribution! :)

I didn't look for everything, but isn't there too much things for a single PR? I see dump per domains, global domains filters, but also a new dumper for AssetMapper... is it too much?

Hi, I thought indeed about making two PRs : one for the filtering and another one for the 'per-domain' dump (which is what the AssetMapperDumper does, there won't be any change on the actual webpack-oriented behavior). I made it in one PR because it all came from the same issue, but I can still split the PR if need be.

smnandre · 2024-07-30T20:03:57Z

Hi @maelanleborgne : do you need anything on this one ?

carsonbot added Feature New Feature Status: Needs Review Needs to be reviewed labels Jun 19, 2024

maelanleborgne changed the title ~~Add global domains filters + add "by-domain" translation dumper~~ [Translator] Add global domains filters + add "by-domain" translation dumper Jun 19, 2024

maelanleborgne force-pushed the feature/split-translation-dump-by-domain branch from 683c1ec to 79f0a13 Compare June 19, 2024 13:16

smnandre reviewed Jun 19, 2024

View reviewed changes

Kocal reviewed Jun 19, 2024

View reviewed changes

maelanleborgne changed the title ~~[Translator] Add global domains filters + add "by-domain" translation dumper~~ [Draft] [Translator] Add "by-domain" translation dumper Jun 20, 2024

maelanleborgne marked this pull request as draft June 20, 2024 08:50

Kocal mentioned this pull request Jun 20, 2024

[Translator] Add configuration to filter dumped translations by domain #1930

Merged

1 task

maelanleborgne force-pushed the feature/split-translation-dump-by-domain branch 2 times, most recently from d0b0305 to a370cb7 Compare July 2, 2024 08:59

maelanleborgne added 2 commits July 2, 2024 11:04

Dump translations by domain as modules to use with AssetMapper

9e45e10

Add backslash to sprintf

108ed58

maelanleborgne force-pushed the feature/split-translation-dump-by-domain branch from a370cb7 to 108ed58 Compare July 2, 2024 09:06

Fixed JS CS

9e3f254

maelanleborgne force-pushed the feature/split-translation-dump-by-domain branch from e0be201 to 9e3f254 Compare July 2, 2024 09:52

maelanleborgne changed the title ~~[Draft] [Translator] Add "by-domain" translation dumper~~ [Translator] Add "by-domain" translation dumper Jul 2, 2024

maelanleborgne marked this pull request as ready for review July 2, 2024 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Translator] Add "by-domain" translation dumper #1927

[Translator] Add "by-domain" translation dumper #1927

maelanleborgne commented Jun 19, 2024 •

edited

Loading

smnandre Jun 19, 2024

smnandre Jun 19, 2024

maelanleborgne Jun 19, 2024 •

edited

Loading

smnandre Jun 19, 2024

smnandre Jun 19, 2024

smnandre Jun 19, 2024

smnandre Jun 19, 2024

smnandre Jun 19, 2024

smnandre Jun 19, 2024

maelanleborgne Jul 2, 2024

smnandre Jun 19, 2024

maelanleborgne Jun 19, 2024

smnandre Jun 19, 2024

Kocal left a comment

maelanleborgne commented Jun 19, 2024

smnandre commented Jul 30, 2024

[Translator] Add "by-domain" translation dumper #1927

Are you sure you want to change the base?

[Translator] Add "by-domain" translation dumper #1927

Conversation

maelanleborgne commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maelanleborgne Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kocal left a comment

Choose a reason for hiding this comment

maelanleborgne commented Jun 19, 2024

smnandre commented Jul 30, 2024

maelanleborgne commented Jun 19, 2024 •

edited

Loading

maelanleborgne Jun 19, 2024 •

edited

Loading