-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
[Translator] Add Symfony UX Translator package #616
Conversation
eb14fb3
to
b7dca2b
Compare
@@ -0,0 +1,149 @@ | |||
/* |
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.
For this file, I've reused the same tests data from https://github.com/symfony/symfony/blob/015d5015e353ee5448bf7c350de0db4a03f8e13a/src/Symfony/Contracts/Translation/Test/TranslatorTest.php
@@ -0,0 +1,60 @@ | |||
/* |
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.
For this file, I've reused some of the same test data from https://github.com/symfony/symfony/blob/015d5015e353ee5448bf7c350de0db4a03f8e13a/src/Symfony/Component/Translation/Tests/Formatter/IntlFormatterTest.php
test('format with HTML inside', function() { | ||
expect(formatIntl('Hello <b>{name}</b>', { name: 'Fab'}, 'messages+intl-icu', 'en')).toEqual('Hello <b>Fab</b>'); | ||
expect(formatIntl('Hello {name}', { name: '<b>Fab</b>'}, 'messages+intl-icu', 'en')).toEqual('Hello <b>Fab</b>'); | ||
}) |
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.
We want to ensure that the issue fixed in willdurand/BazingaJsTranslationBundle#325 is not present here.
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.
Hello 👋 ,
I'm not used to Symfony at all but wanted to suggest some optimizations on the JS part.
} | ||
}); | ||
return intlMessage.format(parameters); | ||
} |
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.
This function has two drawbacks that you may want to solve:
- it is updating the parent object "parameters" without making a proper copy
- it's unclear from your regex what is the expected behavior for cleaning
{
and}
because here you're only looking for open brackets to trigger the clean - (also the if is not really needed if you use a regex)
I'm suggesting below a different approach (you may use the test scenarios for the unit tests too):
function sample(parameters = {}) {
Object.entries(parameters).forEach(([key, value]) => {
if (key.includes('%') || key.includes('{')) {
delete parameters[key];
parameters[key.replace(/[%{} ]/g, '').trim()] = value;
}
});
return parameters
}
function rework(parameters = {}) {
return Object.entries(parameters).reduce((acc, [key, value]) => {
return {
...acc,
[key.replace(/[%{} ]/g, '').trim()]: value
}
}, {});
}
const parameters = {
'example1': 'content1',
'example2%': 'content2',
'{example3}': 'content3',
'%{ example4 }%': 'content4',
'example5 }': 'content5',
'example6% }': 'content6'
}
console.log('params', parameters);
console.log('rework', rework(parameters));
console.log('params', parameters);
console.log('sample', sample(parameters));
console.log('params', parameters);
I made a quick JSFiddle to show the result: https://jsfiddle.net/p6qnzs7t/18/
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.
- Fixed, thanks
- As said on Discord (:eyes:), the logic is taken from the Symfony Translator in its PHP version (https://github.com/symfony/symfony/blob/6.3/src/Symfony/Component/Translation/Formatter/IntlFormatter.php#L45), so I prefer keep the same logic instead.
- Usually you want to quickly check if one of the concerned character can be found before using the regex, to prevent performances loss if you directly use (parse + run) the regex. However I don't know how its behave with JavaScript.
fde423a
to
380b106
Compare
While I can see the need for this, my app doesn't have so many translations that I'd be concerned with the perf impact of dumping the entire catalog. Should we include this functionality here or are you thinking, in this case, use BazingaJsTranslationBundle? |
Is there an alternate way we can flag these translations for js? Adding each and every key in your twig template feels a bit cumbersome, no? Message catalog metadata? Just thinking out loud.
So should we consider moving it's primary functionality here (in addition to this new feature)?
I agree but clearly many people use Bazinga in it's current form w/o the perf issues you describe. I think we should provide both options. |
I can looks cumbersome yeah, but I don't find it shocking since we are not building an SPA. Where the end-user can access to Catalogue Metadata? To me those metadata are internals to Symfony right?
To me, yes. Each PHP + JS existing bundles should be put under the Symfony UX brand/repository.
I think that many people use Bazinga because it's the only one bundle to make re-using Symfony translations messages in JavaScript possible. For performance issues, I'm willing to bet my arm they either don't know or they don't care about these. Unfortunatelly, not every developer use profiling tools like Blackfire for PHP, and the tabs Analyzer/Performance Insights for JavaScript.
We can, this is something I planned for a 2nd PR if people had the issues. We can do it by making the 1st parameter of |
I think it can be used outside - the standard
This, or I was thinking dumping a js file to be included in a user's encore asset build? I use For translations, we'd need the figure out what the current locale is. Can we use the |
I've also used this pattern (with Bazinga and
The |
Certainly Maybe we shouldn't even provide this. If you want to pick and choose which messages to include, use |
Mmmh, maybe it can works if you use |
Yeah, my thinking was to default the locale to the |
380b106
to
9ff2432
Compare
Just my adding my two cents: I did something similar in the past (declaring translations in JS globally) but in the end, I reverted back to translations local to controllers. It's cumbersome to declare all translations to expose so my thinking was that I might as well declare them locally in a "labels" key in the controller to pass the translations directly from Twig: {{ stimulus_controller('my_controller', {
'labels': {
'location': 'crm.list.location'|trans,
'createdAt': 'crm.list.createdAt'|trans,
...
}
) }} This provides the following benefits:
So I'd say that if we don't have a big DX improvement using this component over this "local key" approach, we shouldn't release it. There is a different way I can think of that could provide such great DX improvement: leveraging Webpack tree shaking to not have to care about marking translations to expose and easily importing them. In such case, a translations module would be declared dynamically as a virtual module and would create one exported JS constant per translation key. We could then import these keys in our JS files (we could even have autocompletion if we create a stub file) and they would be linked to either EN or FR at runtime. Because of tree shaking, Webpack would remove the unused keys from the built JS automatically. WDYT? I know it's very different from the current proposal but I really think we should be careful about the DX of this component, it's tricky to do right :) . |
Hi @tgalopin, and thanks for your comment. Your approach is interesting, however I can see three major issues in your code example:
I like this suggestion, however I have many questions:
Thanks! |
I was thinking about a virtual module or a Webpack loader so that it's dynamic and it doesn't need to be dumped in between. It could generate multiple files (one per domain) like: export const CRM_LIST_LOCATION = {
'en': '...',
'fr': '...',
}; This would solve your 3 first questions (2: one file per domain, 3: we need to have locales in the key so that tree-shaking works as expected). For 4, the usage could then be: CRM_LIST_LOCATION['en']
// or perhaps
trans(CRM_LIST_LOCATION, 'en') There may be a better DX to be found though, expecially given how big the DX improvements can be. |
About the virtual module, would it add too much complexity? I'm not familiar with them, but I wonder how we will deal with auto-completion in IDE? With TypeScript support? How do we get translations, from a Symfony command? If yes how do we run it (which If we go to this direction, I prefer having a dedicated Symfony command that will dump translations (for all domains + enabled locales) somewhere in multiple After running the command manually (ie. during a import { trans } from '@symfony/ux-translator';
import { CRM_LIST_LOCATION, FOO_BAR } from '@symfony/ux-translator/translations/messages';
trans(CRM_LIST_LOCATION, { param1: 'value1' }, 'en'); We can also imagine, maybe for later, thanks to TypeScript types, to have auto-completion on translations parameters given the translation id, WDYT? /cc @weaverryan @kbond |
That'd be really cool! Could the imported messages be js objects? (I have minimal experience with js so not sure if this is even possible) import { CRM_LIST_LOCATION, FOO_BAR } from '@symfony/ux-translator/translations/messages';
CRM_LIST_LOCATION; // no parameters just is the value
FOO_BAR.with({ param1: 'value1' });
FOO_BAR.with({ param1: 'value1' }).locale('fr');
// maybe this would be required? (i'm thinking about __toString() in PHP above)
CRM_LIST_LOCATION.render();
FOO_BAR.with({ param1: 'value1' }).render();
FOO_BAR.with({ param1: 'value1' }).locale('fr').render(); Also, how are you thinking the locale-detection would work? I'm thinking the following cascade:
|
The following syntax would be possible, however I'm not a big of creating multiple objects like this because it can lead to performances issues. |
Ok, fair enough.
import { trans } from '@symfony/ux-translator';
import { FOO } from '@symfony/ux-translator/translations/messages';
import { BAR } from '@symfony/ux-translator/translations/domain1';
Nevermind, I see you specified this above :). For small apps that don't have a perf problem loading all their translations, what about something like: import { trans } from '@symfony/ux-translator/translations/_all';
trans('key1');
trans('key2', { param1: 'value1' }, domain: 'security');
trans('key3', domain: 'security', locale: 'fr'); |
The If no, we can't make tree-shaking + translation id auto-completion + translation parameters working all together Constants imported from
Thanks to TypeScript types, we could get parameters autocompletion given the constant name. This is also something that can work with your proposition, but you won't benefit from tree-shaking. If you really need to import all translations, per domain, you can still use WDYT? |
I was thinking yes.
Your probably right but I'm trying to find an easy migration path for bazinga users. |
Alright, I will see what I can do.
Oh, yeah that's true! I don't have a better suggestion for the moment, let's go for |
This is what I've imagined for parameters/locales autocompletion, by using constants: interface Message<Translations = { [locale: string]: string }, Parameters = Record<string, unknown>> {
domain: string;
translations: Translations,
parameters?: Parameters,
}
export type TranslationsOf<T> = T extends Message<infer TranslationsType, infer ParametersType> ? TranslationsType : never;
export type ParametersOf<T> = T extends Message<infer TranslationsType, infer ParametersType> ? ParametersType : never;
function trans<
M extends Message,
Parameters extends ParametersOf<M>,
Translations extends TranslationsOf<M>,
>(message: M, parameters?: Parameters, locale?: keyof Translations): string {
// ...
} With the following constants file, which will be auto-generated by a Symfony command: const CRM_LIST_LOCATION: Message<Record<'en'|'fr', string>, { foo: string }> = {
domain: 'messages',
translations: {
en: 'CRM List {foo}',
fr: 'Liste CRM {foo}'
},
}
const FOO_BAR: Message<Record<'en'|'fr'|'it', string>, { foo: string }> = {
domain: 'messages',
translations: {
en: 'Foo bar (en)',
fr: 'Foo bar (fr)',
it: 'qsd',
}
} |
9405f94
to
3c95625
Compare
Hi everyone, and thanks for the reviews! Your comments have been addressed, here are the updates:
Are you speaking about symfony/symfony#48371? Unfortunately, this Translator package now highly relies on bundler's (Webpack or anything else) tree-shaking. I don't know how works ImportMaps works, but if it does not support tree-shaking, then the Translator won't be usable. :/ |
IMO we shouldn't care too much about importmaps for this PR. My reasonning is that I think there are mostly two use cases for import maps (ie. two types of people using them): 1/ in dev environment it allows not to have to wait for webpack to build, meaning a faster iteration loop => we don't care that the whole translator is imported here, as long as it will work the same way when using Webpack. 2/ In prod environment, almost everyone with a slightly big app uses Webpack or another bundler. Webpack isn't only about building, it's also about productivity: many modules allow to inspect the code, to compile it, to provide DX tools, ... I generally compare it to the Symfony container being built: of course we could do without, but CompilerPass are hugely useful. The only two cases where I can see someone using import maps in prod would be for small projects (=> fine to import the whole translator) or for projects having a large app but a tiny bit of front-end (in which case they most likely should just pass the translations from PHP as Stimulus values or similar). I don't see a case where import maps are an issue, especially given how the DX is so good using the tree-shaking approach (you simply don't need to think about translations). BTW I think a good tool to create in the future will be a Router ;) |
Yes, I think this is the key. I would be careful not to make it look like importmaps are not ok for production, because they are - even in large apps. So the point is that, if you're using importmaps, you are likely using Turbo + Stimulus to get your rich frontend, and so can pass translations as values to Stimulus. |
Ooo, like a replacement for https://github.com/FriendsOfSymfony/FOSJsRoutingBundle? |
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.
👍🏻 from me from a high-level perspective! I love the simplicity!
Well, I had the same idea as Titouan, but I didn't want to spoil until this PR was merged 😛 Now that the logic "warm => dump => import + tree-shaking" is mastered, that would be easy to create a UX Router package. |
Exactly my thinking as well :) |
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.
This is a great PR, thanks a lot for your work @Kocal
This PR was merged into the 2.x branch. Discussion ---------- chore: commit yarn.lock | Q | A | ------------- | --- | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT As discussed with `@weaverryan` on Slack, we will now commit the `yarn.lock` to prevent the same issue than #616 (comment) (2nd part). Commits ------- c18e9f2 chore: commit yarn.lock
(the PR has been rebased) |
752d11b
to
7d7023c
Compare
e86b960
to
8376744
Compare
8376744
to
78f3aa7
Compare
Thank you @Kocal! |
This PR was merged into the 2.x branch. Discussion ---------- chore: commit yarn.lock | Q | A | ------------- | --- | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT As discussed with `@weaverryan` on Slack, we will now commit the `yarn.lock` to prevent the same issue than symfony/ux#616 (comment) (2nd part). Commits ------- c18e9f20 chore: commit yarn.lock
Hi everyone, this PR add a new UX Translator package, which allow to re-use the same logic than the Symfony Translator, but in JavaScript.
The Intl ICU syntax is also supported.
Why?
This package is not a revolution, there is already https://github.com/willdurand/BazingaJsTranslationBundle which allows to re-use translations (defined in your Symfony app) in JavaScript.
Loading issues
However, there is one big difference between https://github.com/willdurand/BazingaJsTranslationBundle and this new UX package: the way translations are passed to JavaScript.
To me, Bazinga contains performance issues. If you choose to dump your translations into a separated
.js
or.json
, or if you choose to load translations through a separate HTTP request, in each case you will need to dump and load the whole catalogue (for a given domain), which contains messages that you will not use into the webpage.With the Symfony UX Translator package's logic, you explicitly define which translations you wants to load, making HTTP responses less heavier and/or JavaScript execution faster.
Types definition
Another issue is that Bazinga does not provide types definitions in the same repository.
Instead, you must install
@types/bazinga-translator
package separately, which can lead to some "desynchronization" between the upstream code and types.How
Everything is documented in https://github.com/Kocal/symfony-ux/blob/feat/translator/src/Translator/doc/index.rst, but to be quick:
trans()
can be imported from@symfony/ux-translator
, which:TranslatorInterface#trans()
from Symfony (PHP)<html>
'slang
attribute, and locales fallbacks