-
-
Notifications
You must be signed in to change notification settings - Fork 398
[Translator] Refactor API to use string-based translation keys instead of generated constants #3208
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
Conversation
📊 Packages dist files size differenceThanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
|
||||||||||||
8c34c38 to
afcb2d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a breaking change to the UX Translator component, refactoring the API from generated constants to string-based translation keys. This change aligns the JavaScript/TypeScript API more closely with Symfony's PHP translation API, allowing developers to use the exact same translation keys in both environments while maintaining type-safety and autocompletion.
Key changes:
- Replaced global translation functions with a
createTranslator()factory that returns a translator instance - Changed from importing translation constants to using string-based keys directly
- Updated generated files to export a
messagesobject andlocaleFallbacksinstead of individual constants - Removed the need for
@app/translationsand@app/translations/configurationimportmap entries
Reviewed changes
Copilot reviewed 29 out of 32 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/Translator/src/TranslationsDumper.php |
Refactored to generate a single messages object instead of individual constants; removed constant name generation logic |
src/Translator/assets/src/translator_controller.ts |
Replaced global functions with createTranslator() factory function returning scoped translator instance |
src/Translator/assets/src/types.d.ts |
Extracted type definitions into separate file; added Messages and MessageId types |
src/Translator/assets/test/unit/translator_controller.test.ts |
Updated all tests to use new createTranslator() API pattern |
src/Translator/tests/TranslationsDumperTest.php |
Updated test assertions to match new output format with messages object |
src/Translator/doc/index.rst |
Updated documentation to reflect new API usage and removed AssetMapper warnings |
src/Translator/CHANGELOG.md |
Added comprehensive changelog entry explaining the breaking change |
ux.symfony.com/templates/ux_packages/translator.html.twig |
Updated demo to use lowercase snake_case translation keys instead of UPPER_CASE constants |
ux.symfony.com/assets/translator.js |
Refactored to use createTranslator() with direct import from var/translations |
ux.symfony.com/importmap.php |
Removed @app/translations entries as they're no longer needed |
apps/encore/assets/translator.js |
Updated to new API pattern with createTranslator() |
apps/e2e/assets/translator.js |
Updated to new API pattern and exports trans from main app file |
apps/e2e/templates/ux_translator/*.html.twig |
Converted all template examples from constants to string-based keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
afcb2d9 to
a8de99d
Compare
| '@app/translations' => [ | ||
| 'path' => './var/translations/index.js', | ||
| ], | ||
| '@app/translations/configuration' => [ | ||
| 'path' => './var/translations/configuration.js', | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for these lines anymore! 🚀
| return { | ||
| setLocale, | ||
| getLocale, | ||
| setThrowWhenNotFound, | ||
| trans, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm returning an object of methods instead of creating a Translator instance (implying this class exists), because:
- I want users being able to use
const { trans } = createTranslator(), with an instance it does not work becausethiscontext is lost - It looks like a bit to the React Hooks or Vue Use systems, which has been pretty common for years in the JS ecosystem
| If you're using WebpackEncore, install your assets and restart Encore (not | ||
| needed if you're using AssetMapper): | ||
|
|
||
| .. code-block:: terminal | ||
| $ npm install --force | ||
| $ npm run watch | ||
| .. note:: | ||
|
|
||
| For more complex installation scenarios, you can install the JavaScript assets through the `@symfony/ux-translator npm package`_ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this part, into a dedicated section below
| import { trans, getLocale, setLocale, setLocaleFallbacks } from '@symfony/ux-translator'; | ||
| import { localeFallbacks } from '../var/translations/configuration'; | ||
| import { createTranslator } from '@symfony/ux-translator'; | ||
| import { messages, localeFallbacks } from '../var/translations/index.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, the index.js here is important, that's for the AssetMapper
| Q&A | ||
| --- | ||
|
|
||
| When installing with AssetMapper, Flex will add a few new items to your ``importmap.php`` | ||
| file. 2 of the new items are:: | ||
| What about bundle size? | ||
| ~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| '@app/translations' => [ | ||
| 'path' => 'var/translations/index.js', | ||
| ], | ||
| '@app/translations/configuration' => [ | ||
| 'path' => 'var/translations/configuration.js', | ||
| ], | ||
| All your translations (extracted from the configured domains) are included in the generated ``var/translations/index.js`` file, | ||
| which means they will be included in your final JavaScript bundle). | ||
|
|
||
| These are then imported in your ``assets/translator.js`` file. This setup is | ||
| very similar to working with WebpackEncore. However, the ``var/translations/index.js`` | ||
| file contains *every* translation in your app, which is not ideal for production | ||
| and may even leak translations only meant for admin areas. Encore solves this via | ||
| tree-shaking, but the AssetMapper component does not. There is not, yet, a way to | ||
| solve this properly with the AssetMapper component. | ||
| However, modern build tools, caching strategies, and compression techniques (Brotli, gzip) | ||
| make this negligible in 2025. Additionally, a future feature will allow filtering dumped | ||
| translations by pattern for those who need to further reduce bundle size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this part was important, since one of my primary concern when creating UX Translator, was about tree-shaking etc.
| private function generateConstantName(string $translationId): string | ||
| { | ||
| $translationId = s($translationId)->ascii()->snake()->upper()->replaceMatches('/^(\d)/', '_$1')->toString(); | ||
| $prefix = 0; | ||
| do { | ||
| $constantName = $translationId.($prefix > 0 ? '_'.$prefix : ''); | ||
| ++$prefix; | ||
| } while ($this->alreadyGeneratedConstants[$constantName] ?? false); | ||
|
|
||
| $this->alreadyGeneratedConstants[$constantName] = true; | ||
|
|
||
| return $constantName; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't make Blackfire profiles, but removing this code will surely help to improve performances when dealing with a ton of translations.
| missing_import_mode: strict | ||
|
|
||
| when@prod: | ||
| framework: | ||
| asset_mapper: | ||
| missing_import_mode: warn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied symfony/recipes#1347
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backported changes from 3.x: c96987f
Configuring the Application to not auto-exit anymore fixed an issue where PHPUnit exited. Thus, no tests were run.
|
I’m speechless. Thank you 🙇 |
This pull request introduces a breaking change to the UX Translator component, refactoring its API to use string-based translation keys instead of generated constants.
The main advantages are:
Before:
After:
Changing the import from
@app/translationsto../var/translations/index.jsallows IDEs and other tools to correctly interpret these files, which is required to maketrans()type-safe.It also make the configuration easier for AssetMapper users, they must now remove these entries from their
importmap.php: