Skip to content

refactor: jsx named placeholders, reuse methods, fix readability#210

Merged
andrii-bodnar merged 14 commits intomainfrom
refactor/jsx-named-placeholders2
Apr 21, 2026
Merged

refactor: jsx named placeholders, reuse methods, fix readability#210
andrii-bodnar merged 14 commits intomainfrom
refactor/jsx-named-placeholders2

Conversation

@timofei-iatsenko
Copy link
Copy Markdown
Collaborator

@timofei-iatsenko timofei-iatsenko commented Apr 20, 2026

This PR improves readability of the added feature, reuse some existing methods etc. Also i extracted tests into separate file to split the concepts.

I updated readme, because found some outdated info there.

UPD: this PR get a bit bigger then expected

I replaced an SWC based snapshot testing framework to Insta.

I was not satisfied with the fact that snapshot has only an output code. That make a reviewing a snapshots pretty inconvinient because to understand is the snapshot generated correctly or not you need to also look into source code which generated that snapshot. Now the snapshotting framework capture:

input code + output code + macro options if they differs from defaults.

---
source: tests/jsx_named_placeholders.rs
info:
  jsx_placeholder_attribute: _t
---
import { Trans } from "@lingui/react/macro";
<Trans>
  Hello <strong _t="em">world</strong>!
</Trans>;

↓ ↓ ↓ ↓ ↓ ↓

import { Trans as Trans_ } from "@lingui/react";
<Trans_ {.../*i18n*/ {
    id: "6N0Fej",
    components: {
        em: <strong/>
    },
    message: "Hello <em>world</em>!"
}}/>;

I also improved a to_panic macro, so it also captures error payload in snaphot. Previously it just testing the fact that transform paniced.

---
source: tests/jsx_named_placeholders.rs
info:
  jsx_placeholder_attribute: _t
---
import { Trans } from '@lingui/react/macro';
<Trans><a _t="same" href="/" {...spread}>A</a> <a _t="same" {...spread} href="/">B</a></Trans>

↓ ↓ ↓ ↓ ↓ ↓

error: Multiple distinct JSX elements with the same placeholder name (`same`). Differentiate them by adding/modifying the `_t` attribute (e.g. `<element _t="newName" />`).
 --> input.tsx:2:48
  |
2 | <Trans><a _t="same" href="/" {...spread}>A</a> <a _t="same" {...spread} href="/">B</a></Trans>
  |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Side changes:

I added claude.md + skill for rust + some project overview claude crunched from the codebase and swc crates.

@timofei-iatsenko timofei-iatsenko marked this pull request as draft April 20, 2026 10:54
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

❌ Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.16%. Comparing base (aa676a1) to head (3a836c6).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ast_utils.rs 97.43% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   93.50%   94.16%   +0.65%     
==========================================
  Files           8        8              
  Lines        1740     1731       -9     
==========================================
+ Hits         1627     1630       +3     
+ Misses        113      101      -12     
Flag Coverage Δ
unittests 94.16% <98.50%> (+0.65%) ⬆️

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

Files with missing lines Coverage Δ
src/builder.rs 98.85% <100.00%> (+1.59%) ⬆️
src/options.rs 100.00% <100.00%> (ø)
src/ast_utils.rs 90.55% <97.43%> (+7.57%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@timofei-iatsenko timofei-iatsenko marked this pull request as ready for review April 20, 2026 12:05
@andrii-bodnar andrii-bodnar requested a review from Copilot April 21, 2026 09:12
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the JSX named placeholder feature implementation for readability/reuse, splits the JSX named placeholder tests into a dedicated integration test file, and updates README compatibility documentation formatting.

Changes:

  • Extract JSX named placeholder tests from tests/jsx.rs into tests/jsx_named_placeholders.rs with corresponding new snapshots.
  • Refactor JSX placeholder handling in MessageBuilder to reuse new AST utility helpers (omit_jsx_attrs, is_jsx_elements_equal).
  • Reformat/clean up README sections (notably the SWC compatibility table formatting).

Reviewed changes

Copilot reviewed 9 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/jsx_named_placeholders.rs New dedicated test suite for JSX named placeholders, including new cases like string-literal JSX expression placeholders.
tests/jsx.rs Removes JSX named placeholder tests now covered by the new test file.
tests/__swc_snapshots__/tests/jsx_named_placeholders.rs/* Adds snapshots for the extracted/updated named placeholder test cases.
tests/__swc_snapshots__/tests/jsx.rs/* Removes snapshots corresponding to the moved named placeholder tests.
src/builder.rs Refactors placeholder attribute extraction and JSX element equality comparison to use shared helpers.
src/ast_utils.rs Introduces helper utilities for omitting JSX attrs and comparing JSX opening elements.
README.md Updates formatting/structure around configuration notes and the compatibility table.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/jsx_named_placeholders.rs Outdated
},
r#"
import { Trans } from '@lingui/react/macro';
<Trans><em _t="same" class="hello">A</em> and <em _t="same" class="hello" data-testId="bla">B</strong></Trans>
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSX in this test has mismatched closing tags (<em ...>B</strong>). This likely causes the test to panic due to a TSX parse error rather than the intended “same placeholder, different attributes count” plugin validation. Fix the closing tag so the input remains valid TSX and the panic originates from the plugin logic being tested.

Suggested change
<Trans><em _t="same" class="hello">A</em> and <em _t="same" class="hello" data-testId="bla">B</strong></Trans>
<Trans><em _t="same" class="hello">A</em> and <em _t="same" class="hello" data-testId="bla">B</em></Trans>

Copilot uses AI. Check for mistakes.
Comment thread tests/jsx_named_placeholders.rs Outdated
Comment thread tests/jsx_named_placeholders.rs Outdated
Comment thread src/ast_utils.rs
Comment on lines +140 to +144
} else {
a.attrs
.iter()
.all(|a| b.attrs.iter().any(|b| a.eq_ignore_span(b)))
}
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_jsx_elements_equal can return a false positive when there are duplicate attributes: the all(|a| any(|b| ...)) check doesn’t account for multiplicity (e.g. a has two identical attrs, b has one identical + one different). Consider comparing attributes as multisets (e.g. match-and-remove from a mutable copy of b.attrs, or sort+compare with eq_ignore_span) when there are no spread elements.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current implementation is good enough as long as test passing. Could not imagine a valid case where this would be a problem.

Comment thread src/builder.rs Outdated
@timofei-iatsenko timofei-iatsenko marked this pull request as draft April 21, 2026 10:06
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 198 out of 200 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/builder.rs
@timofei-iatsenko timofei-iatsenko marked this pull request as ready for review April 21, 2026 13:30
@andrii-bodnar andrii-bodnar merged commit 60c6034 into main Apr 21, 2026
8 checks passed
@andrii-bodnar andrii-bodnar deleted the refactor/jsx-named-placeholders2 branch April 21, 2026 14:44
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.

3 participants