feat(macro): support transforming member expressions to named args #1874
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dan-dr
changed the title
size-limit report 📦
|
|
I thought about something more generic, what about adding a special macro t`Very ${ph(result.cool, 'myName')} Stuff` // -> Very {myName} StuffThis would also work for evey other type of the expressions: // call expression
t`Very ${ph(callExpression(), 'myName')} Stuff`
// this expression
t`Very ${ph(this.myVar, 'myName')} Stuff`Does it make sense? |
|
I think that kinda ruins readability and DX, the variable identifier chain should already be descriptive enough. |
|
By the way, this is valid JS: {
'app.foo.bar': "World"
}As well as this is valid ICU: So may be if it's a MemberExpression just get it as is, without replacing |
|
Additionally this change will affect:
|
right, can be done. maybe also should be configured. I was thinking translators will have easier time with underscore. btw my implementation ignores the regarding the rule, I think it shouldn't effect it? developers should decide if they want it or not, especially since this will be opt-in due to breaking change. what's the stance on swc implementations? it's already diverging due to the |
less configuration, less branching, less maintenance burden. I don't see any benefit of
i saw this, also I think it's obvious and expected from developers point of view if this
It should affect. The purpose of this rule is enforcing developers NOT to write expressions (including member expressions) in the messages due to lack context information and potential poor quality of the translation. If member expression will be processed as proposed in this PR, this rule should be relaxed to skip such cases. Or configuration added, but i prefer to keep them in sync, so the latest version of eslint plugin is aligned with latest version of the lib.
I'm the author of that swc implemetation. The useLingui is opt-in hook, where change from this PR will affect essential macros. I would prefer to not do a lot of configuration and just include it in lingui 5 as default. SWC implementation should also be ready till that moment. |
|
@dan-dr thank you for the contribution! I see a couple of threats here:
|
|
@andrii-bodnar all these points are true for current implementation as well. This PR doesn't bring something new, just extending the cases wich macro / extractor could process. There is a tradeoff between passing more information to translator and re-usability of the message. Angular's team, for example, solves this problem a bit differently, they replace arguments in the string into positional placeholders but also saves meta information how these placeholders are mapped. Something like: # ph1: app.foo.bar
# ph2: myFunction()
msgid: Hello {ph1} and {ph2}So translators could get the point of what variable is in here by looking into metadata. Currently, if developer want to reuse the string, he should provide the same name for all placeholders, sometimes this might require to introduce an intermediate variable for that: t`Hello ${userName}` // -> Hello {userName}
t`Hello ${getUserName()}` // -> Hello {0}
// Developer may reuse the translation by introducing a variable:
const userName = getUserName();
t`Hello ${userName}` // -> Hello {userName} match to the string from case 1So, maybe uncouple variable name from placeholder name not that bad idea even considering worse DX. In my example if developer will specify a placeholder name it would be stable between refactorings and would be more explicit than using a variable name t`Hello ${ph(getUserName(), 'userName')}`
t`Hello ${ph(userName, 'userName')}`By the way, Angular's team solves this issues as well, they have introduce a special magic syntax for that, which is for my test is worse than using additional macro function $localize`Hello ${getUserName()}:userName` // `:userName` after the interpolation is a special "magic" syntax used in Angular to give name for placeholders |
|
Just have got an idea: using this syntax to give a name for a placeholder t`Hello ${{userName: getName()}}`Maybe a little bit weird, but simply fewer symbols to write and minus one import on the top. |
|
I like it a bit more, JS-y. will make it optional. I agree on the eslint rule btw. |
|
I thought about that for a while and here is my conclusion:
In v5 we can make a general configuration which will turn on/off using variable name in placeholder: // useVaraibleNameAsPlaceholder: true (default)
t`Hello ${userName}` // -> Hello {userName}
t`Hello ${user.name}` // -> Hello {user.name}
// useVaraibleNameAsPlaceholder: false
t`Hello ${userName}` // -> Hello {0}
t`Hello ${user.name}` // -> Hello {0} This has more sense than enabling/disabling particular syntax processing. Those users who want as much string as possible to be reused could opt out from named placeholders, or use explicit placeholders syntax The member expression parts should go as-is to the placeholder, no need to replace dots to dashes or cutting levels. t`Very ${this.myVar} Stuff` // -> Very {this.myVar} Stuff
t`Hello ${user.name}` // -> Hello {user.name}
// any level deeper should be supported
t`Hello ${foo.bar.baz}` // -> Hello {foo.bar.baz}
// don't forget about JSX
<Trans>Hello {foo.bar.baz}</Trans> // -> Hello {foo.bar.baz}Also rebase your branch to the |
|
Question: How do translation platforms usually deal with arg changes? Because I think that in v5 you really should make sure people can upgrade and keep the same "behavior". in what you propose, if someone is currently using both |
|
That's why it's planned for v5. We could not go forward if we would need to keep all this backward compatibility. Translation platforms will usually treat these as two separate strings, but with help of Translation Memory the translation could be retrieved back pretty quick. |
|
@andrii-bodnar also not a breaking change if would be under the config flag |
Description
Helps extract complex member expressions to named arguments with
_delimiter.so
t`Very {result.cool} Stuff`results inVery {result_cool} StuffI wanted to add this because we have a lot of results from APIs that I really don't want to break down to different variables.
WIP, wanted to see thoughts of maintainers
TODO:
Types of changes
Checklist