feat: add optional configurable JSX placeholder naming#2505
feat: add optional configurable JSX placeholder naming#2505andrii-bodnar merged 7 commits intolingui:nextfrom
Conversation
|
@mogelbrod is attempting to deploy a commit to the Crowdin Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
a399a85 to
4fd7bd5
Compare
|
@yslpn @Bertg @andrii-bodnar hey, could you share your opinion on the feature design? I think the feature is cool, but i want to make sure that we don't forget anything and the initial design is good to go. |
|
@mogelbrod these new options should also be added to the types/validation in the lingui config. Please also add these new options with a descriptive jsdoc inline comments, explaining how they works and what affects. |
4fd7bd5 to
d71ee97
Compare
d71ee97 to
c74fa3f
Compare
c74fa3f to
e8acb7a
Compare
Thanks for the reminder! This should be done in the most recent push 👍 |
andrii-bodnar
left a comment
There was a problem hiding this comment.
@mogelbrod thanks for the contribution!
It looks good to me. I can't think of any risks or cases that were overlooked. I have left one minor comment.
e8acb7a to
66cc180
Compare
|
@mogelbrod please don't rebase and force push, i'm going to jump on your branch and help with failing snapshots. |
Regarding the feature design, in my personal opinion, it would be better to start with a simple implementation, where the names are taken from the JSX tag name: This will be easier to maintain and more safe. UPD: I've found several very critical issues in the current implementation that are very difficult to detect. Therefore, I would recommend starting with a simpler version. I'm afraid there are still some critical issues I simply haven't found. |
|
|
||
| if (attrIndex !== -1) { | ||
| const attr = attributes[attrIndex] as JSXAttribute | ||
| if (attr.value && attr.value.type === "StringLiteral") { |
There was a problem hiding this comment.
If a custom placeholder attribute is present but its value is not a non-empty string literal, such as
_t={linkName} or _t="",
this code still removes the attribute from the JSX and then silently falls back to the auto-generated numeric placeholder.
Throw an error?
| const existingElement = elementsTracking.get(name) | ||
|
|
||
| if (existingElement) { | ||
| const existingAttrs = existingElement.openingElement.attributes |
There was a problem hiding this comment.
- Both placeholders will be rendered as either
<em />or<strong />depending on the traversal order.
<Trans>
<em _t="same">A</em> and <strong _t="same">B</strong>
</Trans>- The order of attributes isn't compared. For regular props, this often doesn't matter, but for spread, it's important:
const props = { href: "/b" }
<Trans>
<a _t="same" {...props} href="/a">A</a>
<a _t="same" href="/a" {...props}>B</a>
</Trans>That is, the React semantics are different. But the current check sees "there is a spread and there is an href" in both cases and considers the elements identical. After this, one of them quietly wins, and both placeholders begin to render identically, even though the original JSX was different.
There was a problem hiding this comment.
Good catch. However i think this is quite extreme edge case
Thanks for the feedback! Limiting the functionality to only use the component name as placeholder name with no way of overriding it introduces a number of issues. Most notably, it would no longer be possible to use two or more of the same JSX element in a single Replacing one JSX element with another (but otherwise not touching the copy) would also require a re-translation due to the name changing. Eg. Offering both options seems necessary to ensure maintainability and giving developers a tool to work around issues that could otherwise arise. Appreciate the comments @yslpn! I'll get right on addressing the issues you raised. |
Why? We can add indexes just like we did before for |
Ah gotcha - that more closely mirrored the original behaviour of this PR. But it also suffers from the same problem as the current placeholders ( |
Taking the name from a just a JSX element tag name has a little value in my opinion. From my experience usually only simple elements inside of the translation strings are used, mostly In most of the projects i worked on, links were implemented in a few different way such as The proposed design allows to set a semantic, stable name for the placeholder which will not depend on the internal implementation. You can swap So basically users would be able to define a default set such as
Appreciate for the digging deeper in this PR code. I think the initial design is OK for me. We can mark this feature as experimental, users don't have to use right away, but those who will start could provide feedback and we can improve all the cases in the future. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #2505 +/- ##
===========================================
+ Coverage 76.66% 89.35% +12.68%
===========================================
Files 81 118 +37
Lines 2083 3373 +1290
Branches 532 1007 +475
===========================================
+ Hits 1597 3014 +1417
+ Misses 375 324 -51
+ Partials 111 35 -76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Man, I love this change. Having translations with sometimes more than 5 tags are a PITA. Some notes:
(Sorry this is a bit messy, but I'm writing this from my phone and don't feel like reformatting) |
Glad to hear it!
The attribute name is completely configurable (and disabled by default), with
Same tags with none/identical attributes reuse the same placeholder name and JSX element in the current implementation. I previously appended a incremented suffix for non-unique elements, but as @timofei-iatsenko pointed out here it's probably better to throw since those cases are likely unintentional and would be better off explicitly named.
Are you referring to the placeholders in the translation string? My guess is most TMSes don't accept spaces in element names as it's typically a delimeter between tag/attributes, but I haven't double checked. IMO it's better to stick with syntax that's as similar to HTML/JSX as possible for maximum compatibility and understandability. |
|
For me the current design looks good. I propose to merge it as-is and start gathering feedback. I think at the current stage without actually trying to work with it in real projects it's hard to anticipate what works good and what needs polishing. |
timofei-iatsenko
left a comment
There was a problem hiding this comment.
The code itself could be structured a little bit better, but it good for now as-is. It LGTM for me.
Let's focus on merging the base of this feature and blocking other PRs - the rest of polishing could be provided as separate PRs
|
Great to hear! Happy to clean up and/or refactor in this PR if I get some pointers, as well as following up on any issues that are reported post merge 👍 |
|
@timofei-iatsenko do you think it's time to update lingui/swc-plugin#207 to mirror this implementation and rebase #2514 against this branch now? |
|
@mogelbrod no, wait while this one would be merged to next, and then update #2514 over the main. You don't need to use rebase and force push (and i even encourage you to not doing this) since we will squash and merge the PR anyway.
I think so |
|
@mogelbrod @timofei-iatsenko let's keep this feature in sync with the SWC implementation. I would like to merge them both and then release the current scope as I hope this will be the last one before the stable v6 release. |
|
Great! Working on aligning the SWC plugin as we speak 👍 |
Description
Add a new feature that allows developers to customize the identifiers of JSX placeholders generated by the
<Trans>macro. This results in placeholder identifiers in extracted messages that are more stable, human-readable, and contextual, instead of the default arbitrary numeric indices (e.g., replacing<0>with<em>or<link>).It is accomplished via two new optional configuration options (via
lingui.config.jsin themacrooptions):jsxPlaceholderAttribute: Allows defining a custom prop name (e.g.,_t) that can be used on components to set an explicit placeholder name locally.jsxPlaceholderDefaults: Allows providing a record of tag names to placeholder names, enabling global default renaming for specific JSX components.SWC implementation at lingui/swc-plugin#207
Types of changes
Fixes #1188
Checklist
Usage
lingui.config.js
globals.d.ts or similarly imported definitions file
source-file.tsx
Disclaimer
Much of this PR was generated by Copilot using the Gemini 3.1 Pro (Preview) model with training/sharing disabled.
Initial prompt used
Plan: Implement JSX named placeholders
Add opt-in support for named JSX placeholders through Lingui configuration (
jsxPlaceholderAttributeandjsxPlaceholderDefaults), replacing numeric indices in extracted messages for better maintainability and readability. Include smart element deduplication that reuses element names if their props are identical, otherwise with direct suffix appended for duplicated names.Steps
jsxPlaceholderAttribute?: stringandjsxPlaceholderDefaults?: Record<string, string>in themacroobject.exampleConfigin makeConfig.ts sojest-validatewill accept them. LetdefaultConfigomit them (or set them toundefined/ empty object).linguiConfigmacro passing them to theMacroJSXinstantiation insideMacroJsxOpts.MacroJsxOptsin macroJsx.ts to acceptjsxPlaceholderAttributeandjsxPlaceholderDefaults.macroJsx.ts(scoped toMacroJsxContextper message evaluation) to track used placeholder names and their original JSX node props.tokenizeElement: In macroJsx.ts, parse out the requested name:jsxPlaceholderAttribute.jsxPlaceholderDefaultsusing the literal tag name.this.ctx.elementIndex().hrefprops), generate a new key by directly appending an incremental numeric suffix to the original name (starting at 2, e.g.a->a2,a3), avoiding an underscore separator. Store it and assign the newly suffixed name to the current element._tattribute, tag-name defaults, deep nesting, prop deduplication (including equal props and differing props), and direct suffix logic.Relevant files
LinguiConfig.macrotype.exampleConfigfor validator compatibility.macroContexttoMacroJSX.x2) element duplication rules.Verification
pnpm testin babel-plugin-lingui-macro.<a>...</a>and<a2>...</a2>.Decisions
x,x2,x3.