Skip to content

fix: align numeric placeholder generation with babel#239

Merged
timofei-iatsenko merged 1 commit into
lingui:mainfrom
mogelbrod:correct-numeric-indices
Jun 22, 2026
Merged

fix: align numeric placeholder generation with babel#239
timofei-iatsenko merged 1 commit into
lingui:mainfrom
mogelbrod:correct-numeric-indices

Conversation

@mogelbrod

@mogelbrod mogelbrod commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

The SWC plugin assigned auto-generated numeric placeholder indices ({0}, {1}, …) differently from the reference Babel/JS macro (@lingui/babel-plugin-lingui-macro). Because message IDs and values keys are derived from these indices, the two toolchains could extract different catalogs for identical source, and thus break translation lookups in projects that mix the JS and SWC tooling.

ph({ name }), {label: value} and bare identifiers carry an explicit name and, in the babel macro, never advance the counter. The SWC plugin derived the next numeric index from values_indexed.len(), which also counted named placeholders, so any unnamed expression following a named one was numbered one too high.

<Trans>
  {ph({ available: 1 })} of <Plural value={zones.length} one="# zone" other="# zones" /> available
</Trans>
message
Before {available} of {1, plural, one {# zone} other {# zones}} available
After / in JS {available} of {0, plural, one {# zone} other {# zones}} available

Fix

Replaced the values_indexed.len()-based index with a dedicated numeric_index counter (next_numeric_index) that is advanced only when an auto-numeric placeholder is actually assigned — never for named placeholders. This mirrors getExpressionIndex.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.99%. Comparing base (300cf86) to head (e08f777).

Files with missing lines Patch % Lines
crates/lingui_macro/src/builder.rs 97.29% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   94.95%   94.99%   +0.03%     
==========================================
  Files          10       10              
  Lines        2597     2637      +40     
==========================================
+ Hits         2466     2505      +39     
- Misses        131      132       +1     
Flag Coverage Δ
unittests 94.99% <98.14%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
crates/lingui_macro/src/jsx_visitor.rs 89.09% <100.00%> (+0.65%) ⬆️
crates/lingui_macro/src/macro_utils.rs 89.26% <100.00%> (+0.05%) ⬆️
crates/lingui_macro/src/builder.rs 98.73% <97.29%> (-0.23%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@timofei-iatsenko

Copy link
Copy Markdown
Collaborator

TODO: we need to add the same test cases for the babel plugin, so the implementation will not diverge in a future.

The first fix is looks good to me. The second is bothering me, because it think the original implementation of the named placeholders went wrong, and now fixes on top of this implementation making everethying quite complicated and not straightforward.

Could you may be split this into 2 PRs, and i happily approve fix for the first one, and think a bit for the solution for the second one.

@mogelbrod mogelbrod force-pushed the correct-numeric-indices branch from e08f777 to 5ffbfd5 Compare June 22, 2026 10:23
@mogelbrod mogelbrod changed the title fix: align numeric placeholder generation with JS implementation fix: align numeric placeholder generation with babel Jun 22, 2026
@mogelbrod

Copy link
Copy Markdown
Contributor Author

@timofei-iatsenko: I've extracted the second bug fix into #241

@timofei-iatsenko timofei-iatsenko merged commit 27e38f2 into lingui:main Jun 22, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants