fix: align numeric placeholder generation with JS implementation#239
Open
mogelbrod wants to merge 2 commits into
Open
fix: align numeric placeholder generation with JS implementation#239mogelbrod wants to merge 2 commits into
mogelbrod wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 andvalueskeys 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.This PR fixes two distinct divergences in how those indices are generated.
Background: how the JS macro numbers placeholders
In the JS macro, a per-message counter (
getExpressionIndex, a simple() => index++) is advanced only when an expression has no name — i.e. inexpressionToArgumentwhen the expression is not an identifier. Two consequences follow, and the SWC plugin violated both:during traversal.
Bug 1 — named placeholders consumed a numeric index
ph({ name }),{label: value}and bare identifiers carry an explicit name and, in the reference macro, never advance the counter. The SWC plugin derived the next numeric index fromvalues_indexed.len(), which also counted named placeholders, so any unnamed expression following a named one was numbered one too high.{available} of {1, plural, one {# zone} other {# zones}} available{available} of {0, plural, one {# zone} other {# zones}} availableFix: 5ffbfd5
Replaced the
values_indexed.len()-based index with a dedicatednumeric_indexcounter (next_numeric_index) that is advanced only when an auto-numeric placeholder is actually assigned — never for named placeholders. This mirrorsgetExpressionIndex.Bug 2 — choice-component
valueindex was not allocated in source orderFor
<Plural>/<Select>/<SelectOrdinal>, the SWC plugin always allocated thevalueexpression's index first, before processing the option attributes (one,other, …). While this is probably more intuitive, the JS implementation instead iterates attributes in source order and allocates the value's index at the position where thevalueattribute actually appears.This only diverges when a non-identifier
valueis not the first attribute and an earlier option contains a numeric-indexed expression — but when it does, the numbers differ:{0, plural, one {{1} glass} other {many}}{1, plural, one {{0} glass} other {many}}With the conventional value-first ordering both implementations already agreed (
value→{0}), so existing snapshots were unaffected.Fix: e08f777
value_pos: the position of thevalueattribute among the cases in source order (0for theplural(value, { … })call form, where the value is always first).push_icuallocates the value's index atvalue_poswhile iterating the cases, so indices are consumed in source order. The value placeholder is still emitted first in the ICU string by rendering each case body into a buffer (viaString::split_off) and assembling{value, format, …cases}at the end.Index allocation (
numeric_index/values_indexed) persists across the split, so only the output position is decoupled from the allocation order.