⚠️ No Changeset found

Latest commit: e473129

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Binary Sizes

Commit: 3a2226b

Merging this PR will degrade performance by 16.63%

❌ 2 regressed benchmarks
✅ 216 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

_{Comparing kdy1/swc-es-transforms-ts-react-near-full-tsc (e473129) with main (12af4a1)}

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

PR Review: feat(es): add TypeScript + React transforms and tsc corpus tests

Summary

This PR adds TypeScript lowering (type-only pruning, as/satisfies/non-null removal, enum IIFE lowering, namespace/module IIFE lowering) and React JSX lowering (automatic + classic runtime) to the swc_es_transforms rewrite pass, along with AST/parser/codegen/visit/minifier extensions for TsNonNullExpr, TsSatisfiesExpr, and type_only import/export metadata. The tsc corpus structural tests validate the transform output is JS-only and re-parseable. All CI checks pass.

Code Quality & Best Practices

Strengths:

• Clean separation of concerns: AST additions, parser changes, codegen, visit, minifier, and transform logic are each in their own crate/file.
• Good documentation on all new AST types (TsNonNullExpr, TsSatisfiesExpr, type_only fields) with doc comments.
• ReactRuntimeState tracks import deduplication and name freshening nicely — avoids collisions with existing bindings.
• The tsc corpus test with assert_program_is_js_only is a strong structural invariant check.

Observations:

1. is_valid_ident_name is ASCII-only (rewrite.rs:776-784): This function only accepts [a-zA-Z_$][a-zA-Z0-9_$]*. Unicode identifiers (e.g., π, café) that are valid JS/TS identifiers will be incorrectly treated as non-identifiers and serialized as computed properties or string keys. This could cause subtle issues for codebases using non-ASCII identifiers. Consider reusing SWC's existing is_valid_ident utility if one exists, or at minimum using char::is_alphabetic() instead of char::is_ascii_alphabetic().

2. import_source default is "react/jsx-runtime" instead of "react": In ReactTransformOptions::default() the import_source is Atom::new("react/jsx-runtime"). However, in the standard React JSX transform (Babel, TypeScript), importSource is just "react" and /jsx-runtime is appended automatically. This is a deviation from standard tooling behavior. If intentional, worth documenting clearly; otherwise this may surprise users porting configs from Babel/tsc.

3. Parser: type keyword disambiguation (parser.rs:374-378): The check self.peek_kind() != TokenKind::Keyword(Keyword::As) to decide if type is a type-only modifier handles import { type as foo } (where type is the imported binding name), but doesn't account for import { type type as foo } (type-only import of a binding literally named type). This is an edge case but worth a comment.

Potential Bugs / Issues

4. lower_optional_chaining uses == null instead of === null || === undefined (rewrite.rs:646-662): The optional chaining lowering produces base == null ? undefined : base, which uses loose equality. While == null captures both null and undefined, this is subtly different from the spec-compliant lowering which typically uses === null || === void 0 or a == null pattern. The current approach is actually correct for most cases, but it will fail for document.all (which == null is true for but === undefined is false). This is a known edge case and the same approach other tools use, so it's fine — just noting for completeness.

5. export type { ... } sets is_type_only on individual specifiers (parser.rs:645): When parsing export type { A, B }, each specifier gets is_type_only: type_only || is_specifier_type_only. This means when type_only is true at the declaration level, individual specifiers are also marked is_type_only. This is slightly redundant but isn't necessarily wrong — just means both ExportNamedDecl.type_only and ExportSpecifier.is_type_only are true for export type { ... }. Consumers should be aware of this.

6. JSX text trimming (rewrite.rs:371-376): text.trim() trims all Unicode whitespace. The JSX spec's whitespace rules are more nuanced (e.g., preserving single spaces between words across line breaks, collapsing runs). Using .trim() here means <div> hello world </div> loses leading/trailing space. This may cause different runtime output compared to Babel/tsc for whitespace-sensitive content.

Performance Considerations

7. Excessive .cloned() calls: Nearly every store.xxx(id).cloned() call clones the entire AST node before inspecting it. This is necessary for the borrow-checker pattern used here but is worth noting as a potential hotspot. The arena-based store means these are relatively cheap, but for very large programs the clone-then-compare-then-write-back pattern multiplies allocations. A future optimization could use a store.xxx_mut(id) directly where possible.

8. collect_program_used_names walks all top-level declarations: This is O(declarations) per rewrite pass. Since the rewrite is iterative (rewrite_once may run multiple passes), this work is repeated. For large programs it may be worth caching the used-names set across iterations.

9. fresh_react_local linear probing (rewrite.rs:83-95): The loop to find a fresh name does HashSet::insert in a loop starting from index 0. In pathological cases with many name collisions this is O(n), but in practice _jsx, _jsxs, _Fragment will almost never collide, so this is fine.

Security Concerns

No security issues identified. The transform operates purely on AST data with no I/O, user-controlled paths, or dynamic evaluation.

Test Coverage

Strengths:

• The tsc corpus test (tsc_corpus.rs) runs against a large set of real-world TypeScript compiler test cases, covering both automatic and classic JSX runtimes.
• assert_program_is_js_only is a thorough structural assertion that checks no TS declarations, TS expressions, JSX elements, or type-only module declarations remain.
• The output is re-parsed to verify syntactic validity.

Gaps:

• No snapshot/fixture tests for transform output: The tsc corpus tests verify structural invariants (no TS/JSX remains, output parses) but don't verify correctness of the emitted JS. For example, there's no test that enum Foo { A, B = 2, C } produces the correct IIFE with Foo[Foo["A"] = 0] = "A" pattern. Consider adding snapshot tests for key transform behaviors (enum lowering, namespace lowering, JSX lowering with various children/spread/fragment patterns).
• No tests for edge cases in type-only import/export pruning: e.g., import { type A, B } from "x" should keep B, export type * from "x" should be removed entirely. These are tested implicitly via the tsc corpus but explicit targeted tests would provide better regression coverage.
• ~35+ skipped tsc cases: The skip list in should_skip_tsc_case is sizeable. While each is presumably due to a known parser limitation, the skip list should ideally be tracked as issues and shrunk over time.

Overall

Solid, well-structured PR that adds significant TypeScript and React transform capabilities. The main areas for improvement are: (1) adding output-correctness snapshot tests for the key lowering paths, (2) the JSX text whitespace handling, and (3) verifying the import_source default matches standard tooling expectations. The code is clean, well-documented, and all CI checks pass.

Reviewed with Claude Code