refactor(es/typescript): Refactor to use VisitMutHook pattern

[kdy1]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 0114d81

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

[claude]

Pull Request Review: Refactor TypeScript transforms to use VisitMutHook pattern

Summary

This is an ambitious architectural refactoring that introduces a composable hook-based pattern for TypeScript transformations. The implementation follows the established swc_ecma_transformer architecture and shows thoughtful design. However, the PR has 35 failing tests (18% failure rate) and several code quality issues that need to be addressed before merging.

---

Critical Issues

1. Logic Bug in exit_module_items ⚠️

Location: crates/swc_ecma_transforms_typescript/src/hooks/transform.rs:134-138

node.retain_mut(|item| {
    let is_empty = item.as_stmt().map(Stmt::is_empty).unwrap_or(false);
    // Remove those folded into Empty
    is_empty || !item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
});

Problem: This logic is contradictory and always evaluates to true:

• If is_empty is true, the expression is true
• If is_empty is false, then !is_empty is true, so the expression is true
• This means no items are ever filtered out

Should be:

node.retain_mut(|item| {
    !item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
});

Or simply use the existing helper that's already called in StripType hook.

---

2. Missing Hook Methods ⚠️

Several visitor methods are missing from hooks that should probably be present:

In TypeScriptVisitMutWithHook:

• Lines 202-220: Missing enter_simple_assign_target, enter_jsx_element_name, enter_jsx_object, enter_object_pat_prop
• These only have exit hooks but no corresponding enter hooks
• Inconsistent with the pattern used for other nodes

In hook implementations:

• Some hooks implement only exit methods without enter (e.g., exit_export_default_expr in strip_type.rs)
• While not necessarily wrong, this should be documented as intentional

---

3. Duplicate Type Annotation Stripping 🔄

Type annotations are being stripped in two places:

1. In TypeScriptVisitMutWithHook (lines 225-286 in typescript_visitor.rs)
2. In StripTypeHook (lines 56-270 in strip_type.rs)

This is redundant and may cause subtle bugs. The hooks approach should centralize this logic in one place. The visitor should only handle the visitor pattern, not the transformation logic.

Recommendation: Remove type annotation stripping from TypeScriptVisitMutWithHook and rely only on the StripTypeHook.

---

Code Quality Issues

4. Dead Code 📦

The compiler warnings show unused code:

• EnumMemberItem struct and its methods (is_const, build_assign)
• ModuleId trait
• id_to_var_declarator function
• get_enum_id function
• get_member_key function

Action: Remove unused code or document why it's being kept for future use.

---

5. Missing Documentation 📝

The new architecture lacks comprehensive documentation:

• No module-level documentation explaining the hook composition pattern
• TypeScriptCtx struct fields need doc comments
• The relationship between hooks and their execution order is not documented
• No explanation of when to use enter_* vs exit_* hooks

Recommendation: Add doc comments to:

• crates/swc_ecma_transforms_typescript/src/hooks/mod.rs (architecture overview)
• TypeScriptCtx and its state structs
• Each hook's purpose and responsibilities

---

6. Hook Composition Macros 🔧

Location: hook_utils.rs

The macros optional_method! and chained_method! generate a lot of boilerplate for 30+ methods. While functional, this approach:

• Makes debugging harder (macro-generated code)
• Requires manual updates when new hook methods are added
• Could potentially use proc macros instead for better type safety

Suggestion: Consider using proc macros or documenting the trade-offs of this approach.

---

Performance Considerations

7. Clone Operations in Context Initialization ⚡

Location: context.rs:30-48

transform: TransformState {
    import_export_assign_config: config.import_export_assign_config.clone(),
    // ...
}

Multiple fields are cloned from Config during context initialization. Since TypeScriptCtx is created per-transform invocation, these clones happen frequently.

Recommendation: Consider wrapping shared config in Rc or Arc if profiling shows this is a bottleneck.

---

8. Vec Allocation in Class Members 📊

Location: transform.rs:156-173

The transform collects parameter properties into vectors (in_class_prop, in_class_prop_init) even when empty. Most classes don't have parameter properties.

Optimization: Consider using SmallVec from the smallvec crate to avoid heap allocations for the common case.

---

Architectural Concerns

9. Hook Execution Order 🔄

Location: hooks/mod.rs:23-30

The hook chain is:

1. StripImportExport (optional)
2. StripType
3. Transform

Question: Why does StripImportExport need to run before StripType? This ordering dependency should be documented, as it's not obvious and could lead to subtle bugs if changed.

The hooks visit nodes independently, so if StripImportExport modifies the AST in a way that affects what StripType sees, that's a coupling that should be made explicit.

---

10. State Management 🏗️

Location: context.rs

The TypeScriptCtx contains mutable state for three different hooks. This means:

• All hooks can see and potentially modify state from other hooks
• No clear ownership boundaries
• Risk of coupling between hooks

Alternative approach: Consider using a more structured approach where each hook owns its state and the context only provides read-only shared config.

---

11. Test Coverage ⚠️

Critical: 35/190 tests failing (18% failure rate)

The PR description mentions "remaining failures are edge cases that will be addressed in follow-up commits", but this is concerning:

• constructor_1 - Basic functionality
• namespace_* - Multiple namespace tests failing
• exec_tests__fixture__issue_* - Many regression tests failing
• parameter_properties_with_computed - Parameter property handling

Recommendation:

• Do not merge until test success rate is at least 95% (< 10 failures)
• The failing tests suggest fundamental issues, not just edge cases
• Some failures may be related to the bugs identified above (especially ecmascript parser #1)

---

Security Considerations

12. No Obvious Security Issues ✅

The refactoring doesn't introduce obvious security vulnerabilities. The code:

• Doesn't handle user input directly
• Doesn't perform unsafe operations
• Maintains the same security boundaries as the original code

---

Positive Aspects 👍

1. Excellent Architecture: The hook pattern is clean and composable
2. Good Separation of Concerns: Each hook has a clear responsibility
3. Maintains Backward Compatibility: Public API remains unchanged
4. Follows Project Patterns: Matches the swc_ecma_transformer architecture
5. Type Safety: Leverages Rust's type system effectively

---

Recommendations

Before Merge (Required)

1. ✅ Fix the logic bug in exit_module_items (ecmascript parser #1)
2. ✅ Investigate and fix failing tests - target <10 failures
3. ✅ Remove or document dead code (Transformers for ecmascript #4)
4. ✅ Remove duplicate type annotation stripping (EcmaScript lexer is inefficient #3)
5. ✅ Add documentation for the hook architecture (Handle early errors in ecmascript parser #5)

Follow-up (Suggested)

6. 📝 Add missing enter_* hooks for consistency (Clean up #2)
7. 📝 Document hook execution order and dependencies (Drop travis cargo #9)
8. 📝 Consider performance optimizations (Travis and codecov #7, Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)
9. 📝 Evaluate state management architecture (EcmaScript Simplifier #10)

---

Overall Assessment

Status: ❌ Not Ready for Merge

This is impressive architectural work that significantly improves the codebase structure. The composable hook pattern is well-designed and will make future maintenance easier. However, the 18% test failure rate and critical logic bug make this unsafe to merge in its current state.

The refactoring is sound, but the implementation needs refinement. I recommend:

1. Fix the critical bugs identified
2. Get test passage to >95%
3. Add basic documentation
4. Then merge with a plan to address follow-up items

Estimated effort to address critical issues: 1-2 days

---

Review performed with respect to repository guidelines in CLAUDE.md and AGENTS.md

[github-actions]

Binary Sizes

File	Size
swc.linux-x64-gnu.node	28M (28558792 bytes)

Commit: c264bd8

[codspeed-hq]

CodSpeed Performance Report

Merging #11408 will not alter performance

_{Comparing kdy1/port-ts (0114d81) with main (b7978cc)}

Summary

✅ 138 untouched

[claude]

Code Review Summary

This PR refactors the TypeScript transforms to use the VisitMutHook pattern, which is a significant architectural improvement. However, I've identified several critical issues that need to be addressed before merging.

Critical Issues

🐛 Bug: Incorrect Logic in exit_module_items (transform.rs:132-138)

node.retain_mut(|item| {
    let is_empty = item.as_stmt().map(Stmt::is_empty).unwrap_or(false);
    // Remove those folded into Empty
    is_empty || \!item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
});

Problem: This logic is contradictory and will keep ALL items:

• If item.as_stmt() returns Some(stmt):
  • When stmt.is_empty() == true: returns true || false = true (keeps it)
  • When stmt.is_empty() == false: returns false || true = true (keeps it)
• If item.as_stmt() returns None: returns false || true = true (keeps it)

Fix: Should be:

node.retain_mut(|item| {
    \!item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
});

This bug likely contributes to the 18% test failure rate (34 tests failing).

Architectural Review

✅ Strengths

1. Proper VisitMutHook Pattern: Correctly follows the architecture defined in crates/swc_ecma_transformer/AGENTS.md:

   • ✅ TypeScriptVisitMutWithHook implements VisitMut
   • ✅ Hook subtypes implement VisitMutHook
   • ✅ Single VisitMut implementation with composable hooks

2. Good Separation of Concerns: The hook architecture cleanly separates:

   • StripTypeHook: Removes TypeScript type syntax
   • StripImportExportHook: Handles unused imports
   • TransformHook: Core transformations (enums, namespaces, etc.)

3. Centralized State: TypeScriptCtx provides a single source of truth for all transformation state

4. Backwards Compatibility: Public API (typescript(), strip(), tsx()) remains unchanged

⚠️ Performance Concerns

1. Clone of exported_binding HashMap (context.rs:35):

   export_name: ctx.transform.exported_binding.clone(),

   This clones the entire HashMap on every program visit. Consider using Rc or Arc for shared immutable data.

2. Double Traversal for Collection (transform.rs:28-29):

   node.visit_with(&mut Collector { ctx });

   The code does a full AST traversal for collection before the main transformation pass. This is necessary for const enum inlining, but adds overhead.

3. Multiple as_stmt() Calls: The buggy retain logic calls as_stmt() twice per item (inefficient even aside from the bug).

Code Quality Issues

Memory Management

1. Using Cow<str> instead of &str for Atoms: Per CLAUDE.md, prefer &str over Cow<str> for Atom creation. Check context.rs and transform.rs for any violations.

Hook Composition

1. Complex Macro Definitions (hook_utils.rs): The optional_method\! and chained_method\! macros are 200+ lines. This is maintainable but consider if code generation (similar to generated.rs) would be cleaner.

Test Coverage

• Current Status: 156/190 tests passing (82%)
• Concern: 18% failure rate is significant for a refactoring. The PR description mentions "edge cases will be addressed in follow-up commits" but these should ideally be fixed before merging to avoid breaking main branch.

Recommendation

1. Fix the critical exit_module_items bug
2. Run the test suite and verify that fixes the failures
3. Add specific tests for the module item filtering logic
4. If failures remain, consider marking this PR as draft until all tests pass

Security Review

✅ No security concerns identified:

• No unsafe code blocks
• No command injection vectors
• No XSS or injection vulnerabilities
• Proper use of syntax contexts and marks

Documentation

• ⚠️ Missing inline documentation for complex functions in transform.rs (1513 lines)
• ✅ Good comments explaining TypeScript-specific handling in typescript_visitor.rs
• ⚠️ ExportQuery struct could use more documentation about its purpose

Recommendations

Must Fix Before Merge

1. Fix the exit_module_items retain logic bug in transform.rs:132-138
2. Run full test suite and investigate remaining failures
3. Add unit test specifically for empty statement filtering

Should Fix Before Merge

1. Consider Rc<FxHashMap> instead of cloning exported_binding
2. Add documentation to large functions in transform.rs
3. Run cargo fmt --all as required by CLAUDE.md

Consider for Follow-up

1. Benchmark performance impact of double AST traversal
2. Consider generated code for hook composition macros
3. Add more granular unit tests for individual hooks

Positive Notes

This is excellent architectural work that significantly improves the TypeScript transform codebase. The hook pattern is cleaner, more composable, and easier to understand than the previous monolithic approach. Once the bugs are fixed and tests pass, this will be a major improvement to maintainability.

Great job on the refactoring! 🎉

---

Note: This review focused on the architectural changes and new hook files. The existing TypeScript transformation logic that was moved (but not modified) was not re-reviewed.

[claude]

Code Review for PR #11408

Overview

This PR refactors the TypeScript transform to use the VisitMutHook pattern, aligning with the architecture established in swc_ecma_transformer. The refactoring is substantial (3,281 additions, 161 deletions) and introduces a composable hook-based architecture.

✅ Strengths

Architecture & Design

• Excellent separation of concerns: The refactoring cleanly separates concerns into three distinct hooks (StripImportExport, StripType, Transform), each with focused responsibilities
• Composability: The hook chain pattern with HookBuilder, OrderedChain, and OptionalHook provides flexible composition
• State management: Centralizing state in TypeScriptCtx with dedicated sub-states (TransformState, StripImportExportState, StripTypeState) is clean and maintainable
• Backward compatibility: Public APIs (typescript(), strip(), tsx()) remain unchanged, preserving backward compatibility

Code Quality

• Good documentation: Comments explaining critical behavior (e.g., why visit_mut_opt_accessibility sets to None)
• Proper use of macros: The optional_method! and chained_method! macros reduce boilerplate effectively
• Stack overflow protection: Uses maybe_grow_default for recursive operations

🔍 Issues & Concerns

1. Test Coverage - Critical ⚠️

The PR description states "156/190 tests passing (82%)" initially, improving to "176/190 passing (93%)" after follow-up fixes. 18% of tests still failing is a significant concern for a refactoring PR.

Recommendation:

• All existing tests should pass before merging a refactoring that claims to maintain functionality
• The PR should either:
  • Fix all remaining test failures, OR
  • Clearly document which tests are failing and why (with tracking issues for follow-ups)
  • Add regression tests for the fixes made in follow-up commits

2. Potential Logic Bug in exit_module_items 🐛

In crates/swc_ecma_transforms_typescript/src/hooks/transform.rs:134-138:

node.retain_mut(|item| {
    let is_empty = item.as_stmt().map(Stmt::is_empty).unwrap_or(false);
    // Remove those folded into Empty
    is_empty || !item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
});

This logic appears incorrect:

• Sets is_empty = item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
• Then returns is_empty || !is_empty, which always evaluates to true
• This means nothing gets filtered out

Expected behavior: Should filter out empty statements
Suggested fix:

node.retain_mut(|item| {
    !item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
});

3. Type Visitor Propagation ⚠️

The TypeScriptVisitMutWithHook correctly prevents visiting TypeScript type nodes via:

• noop_visit_mut_type!() macro
• Manual implementations for visit_mut_opt_ts_* methods that don't call visit_mut_children_with

However, there's a subtle issue: The hook methods for types (enter_opt_ts_type, etc.) are called, but hooks cannot visit children of these nodes. This is correct for the current implementation but could be a footgun if future hooks try to inspect type structures.

Recommendation: Add documentation in VisitMutHook trait or context explaining that hooks receive type annotation nodes but cannot traverse their children.

4. Performance Considerations 🚀

Cloning in Context Creation

In TypeScriptCtx::new() (context.rs:30-48):

import_export_assign_config: config.import_export_assign_config.clone(),

The config is cloned in multiple places. For a frequently-called transform, this could add overhead.

Recommendation: Consider using Arc<Config> or restructuring to avoid clones if profiling shows this is a hotspot.

Double Traversal

The Collector visitor in enter_program (transform.rs:29) performs a full AST traversal before the main transform. This is necessary for enum value collection but doubles the traversal cost.

Note: This appears to match the original implementation, so not a regression. Just noting for future optimization.

5. Code Duplication in TypeScriptReactHook 📋

In typescript.rs:87-241, TypeScriptReactHook manually delegates all hook methods to inner_hook. This is a lot of boilerplate (150+ lines).

Recommendation:

• Consider a blanket implementation or macro to auto-delegate methods
• Or use impl VisitMutHook<TypeScriptCtx> for &mut H pattern

Example approach:

macro_rules! delegate_hook {
    ($method:ident, $T:ty) => {
        fn $method(&mut self, n: &mut $T, ctx: &mut TypeScriptCtx) {
            self.inner_hook.$method(n, ctx);
        }
    };
}

6. Missing Documentation 📝

• The hooks module (mod.rs) lacks module-level documentation explaining the hook architecture
• TypeScriptCtx fields are documented but relationships between state components aren't explained
• No examples showing how to extend with custom hooks

Recommendation: Add comprehensive module docs:

//! TypeScript transform hooks
//!
//! This module implements TypeScript transforms using the VisitMutHook pattern.
//! 
//! ## Architecture
//! The transform is split into three composable hooks:
//! 1. `StripImportExport` - Removes unused type imports
//! 2. `StripType` - Removes all TypeScript type syntax
//! 3. `Transform` - Core transforms (enums, namespaces, etc.)
//!
//! ## Hook Composition
//! [examples...]

7. Error Handling ⚠️

The code uses HANDLER.with(|handler| handler.err(...)) in several places but it's not clear:

• Whether errors are accumulated or fatal
• How errors propagate through the hook chain
• What the error recovery strategy is

Recommendation: Document error handling strategy and ensure hooks handle errors consistently.

🔒 Security Considerations

No security issues identified. The refactoring maintains the same transformation logic with better structure.

⚡ Performance

The refactoring appears performance-neutral:

• Same overall algorithm (collection pass + transform pass)
• Hook dispatch overhead should be minimal (likely optimized away)
• State is now heap-allocated in TypeScriptCtx but this is likely negligible

Recommendation: Run benchmarks to confirm no regression, especially on large TypeScript files.

📊 Test Coverage

Beyond the failing tests mentioned above:

• Need to verify all edge cases from the follow-up commits have dedicated tests
• Consider adding tests specifically for the hook composition mechanism
• Add tests for error cases in hook chains

🎯 Recommendations Summary

Must Fix Before Merge:

1. ✅ Fix the exit_module_items logic bug (transform.rs:134-138)
2. ✅ Get all tests passing or document failures with tracking issues
3. ✅ Run benchmarks to confirm no performance regression

Should Fix:

4. Reduce code duplication in TypeScriptReactHook
5. Add comprehensive module and architecture documentation
6. Document error handling strategy
7. Add documentation about type node visiting limitations

Nice to Have:

8. Profile and optimize config cloning if it's a hotspot
9. Add examples for extending with custom hooks
10. Consider extracting hook composition patterns to swc_ecma_hooks

Final Assessment

This is a well-architected refactoring that significantly improves code organization and maintainability. The hook pattern is clean and composable. However, the test failures are a blocker - a refactoring should not break existing functionality. Once tests pass and the logic bug is fixed, this will be a great improvement to the codebase.

Overall: Approve with required changes ⚠️

---

Generated by Claude Code Review

[claude]

Pull Request Review

Overview

This PR refactors TypeScript transforms to use the VisitMutHook pattern, which is a significant architectural improvement. The changes follow the pattern established in swc_ecma_transformer and provide better composability and maintainability.

Positive Aspects ✅

1. Architecture & Design

   • Excellent separation of concerns with dedicated hooks for StripType, StripImportExport, and Transform
   • Clean state management through TypeScriptCtx with clearly separated state per hook
   • Good use of HookBuilder pattern for composable hook chains
   • Maintains backwards compatibility for public APIs (typescript(), strip(), tsx())

2. Code Organization

   • Well-structured modules: context, hook_utils, strip_import_export, strip_type, transform, typescript_visitor
   • Clear separation between collection phase and transformation phase
   • TypeScript type node handling is properly isolated in TypeScriptVisitMutWithHook

3. Implementation Quality

   • Follows SWC conventions (uses Cow<str> over String where appropriate)
   • Proper use of FxHashMap/FxHashSet for performance
   • Stack-based state management for nested scopes (e.g., saved_var_list)

Concerns & Issues 🔍

1. Test Coverage - Critical

The PR description mentions 156/190 tests passing (82%) with 34 failing tests. While the description states "remaining failures are edge cases," this needs more clarity:

• What specific edge cases are failing?
• Are these regressions or pre-existing issues?
• What's the plan to fix them before merge?
• Could any failures indicate correctness issues?

Recommendation: Please provide a detailed breakdown of failing tests. For a refactoring PR, we should aim for 100% test pass rate to ensure no regressions.

2. Potential Memory/Performance Issues

Location: context.rs:54-81 - TransformState

pub struct TransformState {
    pub ref_rewriter: Option<RefRewriter<ExportQuery>>,
    pub ref_rewriter_temp: Option<RefRewriter<ExportQuery>>,
    pub decl_id_record: FxHashSet<Id>,
    pub namespace_id: Option<Id>,
    pub exported_binding: FxHashMap<Id, Option<Id>>,
    pub enum_record: TsEnumRecord,
    pub const_enum: FxHashSet<Id>,
    pub var_list: Vec<Id>,
    pub export_var_list: Vec<Id>,
    pub saved_var_list: Vec<Vec<Id>>,
    pub in_class_prop: Vec<Id>,
    pub in_class_prop_init: Vec<Box<Expr>>,
    // ... many fields
}

Issue: Large state struct is created once and reused. For files with many enums/namespaces, the hash maps and vectors could accumulate significant data.
Recommendation: Consider whether state should be cleared between transformations or document the expected lifecycle.

3. Clone in Hot Path

Location: context.rs:108 and context.rs:121

.clone()
.map(|namespace_id| namespace_id.make_member(export_name.clone().into()).into())

Issue: Multiple clones in query methods that may be called frequently during transformation.
Recommendation: Profile to ensure this doesn't impact performance. Consider if references can be used instead.

4. Default Trait Implementation Concerns

Location: context.rs:52 and context.rs:85-86

#[derive(Default)]
pub struct TransformState {
    pub import_export_assign_config: TsImportExportAssignConfig,
    // ...
}

Issue: TransformState derives Default but has a field import_export_assign_config that should be set from Config. The Default implementation could lead to incorrect behavior if state is accidentally default-initialized.
Recommendation: Remove Default derive and force explicit initialization, or add a comment explaining why Default is safe here.

5. Error Handling

Location: strip_import_export.rs:56-75
The code silently filters imports based on usage, but there's no logging or diagnostics when imports are removed. This could make debugging difficult.
Recommendation: Consider adding debug logging when imports are stripped to aid troubleshooting.

6. Code Duplication

Location: hook_utils.rs has significant macro-generated code
The optional_method\! and chained_method\! macros generate ~400 lines of boilerplate. While this is a reasonable use of macros, it makes the code harder to navigate.
Minor: This is acceptable but worth noting for maintainability.

Security & Safety ✅

• No unsafe code introduced
• No obvious command injection or XSS vulnerabilities
• Proper use of DUMMY_SP and PURE_SP for generated code
• No file system operations or network access

Performance Considerations ⚡

1. Positive: Single-pass architecture should be more efficient than multiple separate passes
2. Positive: Hook composition avoids redundant tree traversals
3. Concern: Two-phase collection (line 29 in transform.rs) means the AST is visited twice
4. Recommendation: Run benchmarks to ensure performance is maintained or improved

Testing Recommendations 🧪

1. Add tests specifically for hook composition
2. Test edge cases around nested namespaces and enum references
3. Test that RefRewriter works correctly with complex export scenarios
4. Add regression tests for the 34 currently failing tests once fixed
5. Consider fuzzing with malformed TypeScript to ensure robustness

Documentation 📝

• Code has good inline comments explaining complex logic
• TypeScriptCtx and hook purposes are well-documented
• Missing: No module-level docs in hooks/mod.rs explaining the hook architecture
• Missing: No documentation on the order of hook execution and why it matters

Style & Conventions ✅

• Follows AGENTS.md guidelines
• Uses English comments and documentation
• No use of unstable/nightly Rust features
• Proper use of Cow<str> and &str for Atom instances

Blocking Issues ⛔

1. Test failures: 34 failing tests must be resolved or thoroughly explained
2. CI Status: All CI checks are pending - we need green CI before merge

Recommendations Summary

Before Merge:

1. ✅ Fix or document all 34 failing tests
2. ✅ Ensure all CI checks pass
3. ⚠️ Consider removing Default derive from TransformState or document why it's safe
4. ⚠️ Add module-level documentation explaining hook architecture
5. ⚠️ Run performance benchmarks to ensure no regression

Future Improvements (can be follow-up PRs):

1. Add debug logging for import stripping decisions
2. Consider state cleanup for long-lived transform instances
3. Profile clone performance in query methods
4. Add fuzz testing for robustness

Verdict

This is a well-architected refactoring that moves the codebase in the right direction. The hook pattern improves composability and maintainability. However, the 18% test failure rate is a blocker that must be addressed before merge. Once tests are fixed and CI is green, this should be good to merge.

Great work on a complex refactoring! 🎉

---

Review conducted following CLAUDE.md and AGENTS.md guidelines

[claude]

Pull Request Review: Refactor TypeScript transforms to use VisitMutHook pattern

Overall Assessment ✅

This is a well-executed architectural refactoring that successfully migrates the TypeScript transforms to use the composable VisitMutHook pattern. The implementation demonstrates solid understanding of the architecture and follows established patterns from swc_ecma_transformer.

Test Status: 187/190 passing (98.4% pass rate) - excellent progress from the initial implementation!

---

Strengths 💪

1. Correct Architectural Pattern

• Properly implements VisitMutHook instead of direct VisitMut implementations ✅
• Follows the repository's AGENTS.md guidelines exactly as specified
• Single VisitMut implementation via TypeScriptVisitMutWithHook
• Subtypes correctly implement VisitMutHook<TypeScriptCtx>

2. Clean Hook Composition

The hook builder pattern in hooks/mod.rs is elegant and composable:

let hook = HookBuilder::new(NoopHook)
    .chain_optional(if !config.verbatim_module_syntax {
        Some(strip_import_export::hook())
    } else {
        None
    })
    .chain(strip_type::hook())
    .chain(transform::hook());

3. Centralized State Management

TypeScriptCtx properly consolidates all transform state:

• TransformState for main transformations
• StripImportExportState for import/export handling
• StripTypeState for type stripping
• Clear separation of concerns

4. Type Node Handling

typescript_visitor.rs correctly prevents visiting TypeScript type annotations:

• Uses noop_visit_mut_type!() macro
• Strips type annotations before visiting children
• Prevents type nodes from being traversed

5. Backwards Compatibility

Public API (typescript(), strip(), tsx()) remains unchanged - excellent!

---

Issues Found 🔍

1. Dead Code ⚠️ (Low Priority)

Several functions/structs in transform.rs are unused:

• EnumMemberItem struct and its methods (is_const, build_assign)
• trait ModuleId
• id_to_var_declarator function
• get_enum_id, get_member_key functions

Recommendation: Remove dead code or mark with #[allow(dead_code)] if intended for future use.

2. Three Failing Tests ⚠️ (Medium Priority)

issue_2886_enum_namespace_block_scoping
issue_960_2 (mocha test harness issue)
parameter_properties_with_computed

Status: The PR description mentions these are "edge cases that will be addressed in follow-up commits." This is acceptable for an incremental refactoring approach, but they should be tracked.

3. Missing Changeset ℹ️

The changesets bot indicates no changeset was added. Since this is a refactor with no public API changes, this is fine, but verify with maintainers if a changeset is needed for internal architecture changes.

---

Code Quality Assessment

Performance ✅

• Uses Cow<str> and &str appropriately for Atoms (as per AGENTS.md)
• Stack growth protection via maybe_grow_default() in recursive functions
• Efficient hash maps (FxHashMap, FxHashSet) for lookups

Documentation ✅

• Good inline comments explaining non-obvious logic
• Struct/function documentation present
• Complex state transitions are commented

Memory Safety ✅

• Proper use of take() for ownership transfer
• No unsafe code introduced
• Correct handling of mutable references in visitor pattern

Maintainability ✅

• Clear module structure (hooks/ directory)
• Good separation between concerns (strip_type, strip_import_export, transform)
• Hook composition makes it easy to add/remove transforms

---

Performance Considerations

Strengths:

1. Single-pass design - All hooks run in one traversal
2. Lazy initialization - ref_rewriter only created when needed
3. Efficient collection - Uses visitor pattern for initial collection phase

No Performance Regressions Expected:

The refactoring maintains the same algorithmic complexity as the original implementation while improving composability.

---

Security Assessment ✅

No security concerns identified:

• No user input parsing changed
• No new unsafe code
• Maintains existing security properties
• Type system prevents common errors

---

Recommendations

Before Merge:

1. ✅ Fix or document the 3 failing tests - Either fix them in this PR or create follow-up issues
2. ⚠️ Remove dead code - Clean up unused functions in transform.rs
3. ℹ️ Add changeset if required - Verify with maintainers

Future Improvements (Optional):

1. Consider adding hook-level documentation explaining the ordering requirements
2. Could extract ExportQuery to a separate module for reusability
3. Performance benchmarks would be valuable to confirm no regressions

---

Conclusion

This is high-quality work that successfully modernizes the TypeScript transform architecture. The 98.4% test pass rate demonstrates thoroughness, and the code follows repository conventions excellently.

Recommendation: Approve with minor cleanup ✅

The remaining 3 test failures and dead code should be addressed, but they don't block the architectural direction. This refactoring provides a solid foundation for future TypeScript transform improvements.

---

Reviewed by: Claude (AI Code Review)
Date: "2025-12-30"
Commit: 99751b3

[claude]

PR Review: Refactor TypeScript Transforms to use VisitMutHook Pattern

Summary

This PR represents a significant architectural refactoring of the TypeScript transforms crate to adopt the VisitMutHook pattern. The changes align with the architecture established in swc_ecma_transformer as documented in crates/swc_ecma_transformer/AGENTS.md.

---

Code Quality & Architecture ✅

Strengths

1. Excellent Hook Composition: The hook architecture is well-designed with clear separation of concerns:

   • StripImportExportHook: Handles import/export removal
   • StripTypeHook: Removes TypeScript type syntax
   • TransformHook: Core transformations (enums, namespaces, parameter properties)
   • Clean composition in typescript_hook()

2. State Management: The TypeScriptCtx struct centralizes all state, making it easier to track and manage context across hooks.

3. Proper Hook Utilities: The OptionalHook and OrderedChain patterns provide composable building blocks following good functional patterns.

4. Backwards Compatibility: Public API functions (typescript(), strip(), tsx()) are maintained, ensuring no breaking changes.

5. Following Project Guidelines: The implementation correctly follows the instructions in crates/swc_ecma_transformer/AGENTS.md:

   • ✅ Only one VisitMut implementation (TypeScriptVisitMutWithHook)
   • ✅ Subtypes implement VisitMutHook<TypeScriptCtx>
   • ✅ Hook-based architecture

---

Performance Considerations ✅

Positive Aspects

1. Single-Pass Architecture: The hook pattern maintains single-pass visiting, which is critical for performance.

2. Efficient State Sharing: Using a shared context (TypeScriptCtx) instead of multiple visitor passes reduces overhead.

3. Memory Efficiency:

   • Uses FxHashMap and FxHashSet for better performance
   • Proper use of Vec stacks for scoped state management
   • Following the codebase guideline: "Write performant code. Always prefer performance over other things."

4. Stack-Based Scoping: The saved_var_list uses a Vec<Vec<Id>> stack pattern (as seen in commit 6f5e6a3) to properly handle nested scopes without cloning.

Suggestions

1. Consider Cow<str> for Atom Creation: Per AGENTS.md, when creating Atom instances, prefer Cow<str> or &str over String. Review places like hooks/transform.rs:76 where member names are converted.

---

Correctness & Testing 🎯

Test Coverage: Excellent (100% passing)

The PR shows progressive improvement through 18 commits:

• Started at 82% (156/190 tests)
• Final state: 100% (5036/5036 tests passing) ✅

Key bug fixes addressed:

• Namespace export transformations and RefRewriter application
• Abstract accessor and declare member filtering
• Optional flag handling in parameter properties
• Ambient module handling (quoted names)
• Setter property this parameter removal
• Nested namespace var_list scoping

Potential Issues

1. Complex RefRewriter Logic: The RefRewriter save/restore logic in nested namespaces (commits 99751b3, ba561d6) is intricate. Key areas:

   • hooks/transform.rs:83-122 (TsNamespaceDecl)
   • hooks/transform.rs:124-171 (TsModuleDecl)

   Consider adding inline comments explaining the state transitions for future maintainers.

2. Ambient Module Detection: The pattern for detecting ambient modules (checking for quoted names) appears in multiple places:

   • hooks/transform.rs:126-128
   • hooks/transform.rs:142-144
   • hooks/strip_import_export.rs:143-149

   Recommendation: Extract this into a helper function to reduce duplication:

   fn is_ambient_module(decl: &TsModuleDecl) -> bool {
       decl.id.is_str()
   }

---

Security Considerations ✅

No security concerns identified. The refactoring:

• Doesn't introduce new external dependencies beyond swc_ecma_hooks
• Maintains existing security boundaries
• Doesn't expose new public API surface

---

Documentation & Comments 📝

Good Practices

1. Commit messages follow a clear pattern with test result tracking
2. The PR description provides clear benefits and architectural rationale

Areas for Improvement

1. Add Module-Level Documentation: The new hooks/ module files would benefit from doc comments explaining:

   • The overall hook execution order
   • The relationship between hooks and the visitor
   • Example usage patterns

2. Complex Algorithm Comments: Add comments for non-obvious logic:

   • RefRewriter save/restore in nested namespaces
   • Why ref_rewriter_temp is needed alongside ref_rewriter
   • The var_list stack management

Per AGENTS.md: "Write documentation for your code."

---

Specific Code Concerns

1. saved_namespace_id Type (hooks/context.rs:67)

pub saved_namespace_id: Option<Option<Id>>,

The nested Option is correct but could be documented. It represents:

• None: No save operation performed
• Some(None): Saved state was "no namespace"
• Some(Some(id)): Saved state was "in namespace id"

Consider a doc comment or a more semantic wrapper type.

2. Macro Repetition (hooks/hook_utils.rs)

The optional_method! and chained_method! macros reduce boilerplate well. However, the repetitive method calls (67 node types) might indicate this could be generated code. If not already, consider code generation for maintenance.

3. collector_saved_namespace_id (hooks/context.rs:77)

This field appears unused in the diff. Verify if it's needed or should be removed.

---

Best Practices Compliance

✅ Follows AGENTS.md guidelines:

• Uses proper Rust style and formatting
• No unstable rustc features
• Comments in English
• Tests not removed/modified incorrectly

✅ Follows swc_ecma_transformer pattern:

• Single VisitMut implementation
• Hooks implement VisitMutHook<TraverseCtx> (here TypeScriptCtx)
• Proper hook execution through VisitMutHooks

---

Final Assessment

Strengths

• Clean architectural refactoring with clear separation of concerns
• Excellent progressive debugging approach (18 commits to 100% tests)
• Maintains backwards compatibility
• Follows project conventions and patterns
• Performance-conscious design

Recommendations for Merge

Before merging, consider:

1. ✅ Run cargo fmt --all (as per AGENTS.md)
2. Add documentation comments to the new hooks/ module
3. Extract is_ambient_module helper to reduce duplication
4. Document the saved_namespace_id: Option<Option<Id>> pattern
5. Verify collector_saved_namespace_id is needed

Overall: Strong approval ✅ pending minor documentation improvements. The refactoring achieves its goal of adopting the VisitMutHook pattern while maintaining correctness and performance. The 100% test pass rate demonstrates thorough validation.

---

Test Status: ⏳ CI checks are still pending. Please ensure all checks pass before merging.

Great work on this substantial refactoring! The progressive approach through multiple commits made the complex transformation manageable. 🎉

[claude]

Code Review: TypeScript Transforms Refactor to VisitMutHook Pattern

I've reviewed this PR and here's my comprehensive feedback:

✅ Strengths

1. Architectural Alignment: The refactor successfully follows the swc_ecma_transformer pattern as specified in AGENTS.md:

   • Single VisitMut implementation via TypeScriptVisitMutWithHook
   • Subtypes implement VisitMutHook<TypeScriptCtx> (not VisitMut)
   • Proper hook composition via HookBuilder

2. Clean Separation of Concerns:

   • StripTypeHook: Removes TypeScript type syntax
   • StripImportExportHook: Handles import/export removal
   • TransformHook: Core transformations (enums, namespaces, etc.)
   • Each hook has clear responsibilities

3. Centralized State Management: TypeScriptCtx consolidates all shared state, eliminating scattered mutable state across multiple visitor implementations.

4. Type Safety: The hook chain is enforced at compile-time via the type system, preventing composition errors.

⚠️ Critical Issues

1. Test Failures (82% Pass Rate)

The PR description mentions 156/190 tests passing (82%). While the author notes these are "edge cases," 18% failure rate is significant for a refactor. Key concerns:

• Module namespace scoping changes: Multiple test outputs show different variable scoping (e.g., X.Y vs Y). Example from TwoInternalModulesThatMergeEachWithExportedAndNonExportedClassesOfTheSameName.1.normal.js:

  -    })(X.Y || (X.Y = {}));
  +    })(Y || (Y = {}));
  +export var Y;

• Semantic differences: The namespace scoping changes could break runtime behavior in complex module scenarios.

Recommendation: These test failures should be fixed before merging, not deferred. They indicate potential bugs in namespace/module handling logic.

2. Type Annotation Stripping in typescript_visitor.rs

Lines 256-365 manually strip type annotations by setting fields to None before visiting children:

fn visit_mut_class_prop(&mut self, node: &mut ClassProp) {
    node.type_ann = None;  // Manual stripping
    node.visit_mut_children_with(self);
    self.hook.exit_class_prop(node, &mut self.context);
}

Issue: This creates redundancy with StripTypeHook, which also strips type annotations. The dual-layer approach could mask bugs where one layer fails.

Recommendation: Move all type annotation stripping to StripTypeHook for single source of truth. The visitor should only handle traversal ordering.

3. RefRewriter Complexity

transform.rs has complex RefRewriter state management with temporary storage (ref_rewriter_temp, ref_rewriter_saved_for_var_decl). Lines 96-122 show nested namespace handling:

// For nested namespaces, ref_rewriter_temp already contains parent RefRewriter
if ctx.transform.ref_rewriter_temp.is_none() {
    ctx.transform.ref_rewriter_temp = ctx.transform.ref_rewriter.take();
}

Concern: This logic is hard to reason about and may be contributing to the namespace scoping test failures.

Recommendation: Add comprehensive comments explaining the state transitions and when each field is used. Consider simplifying with a stack-based approach.

🔍 Potential Bugs

1. Namespace ID Restoration (transform.rs:121):

   ctx.transform.namespace_id = ctx.transform.saved_namespace_id.take().flatten();

   The .flatten() converts Option<Option<Id>> to Option<Id>, but error handling is unclear if state corruption occurs.

2. Enum Value Mutation (transform.rs:54-80):
   Enum members are collected during enter_ts_enum_decl, but if the AST is mutated during traversal, the enum_record may become stale.

3. Missing visit_mut_children_with in Optional Accessibility (typescript_visitor.rs:63-67):

   fn visit_mut_opt_accessibility(&mut self, node: &mut Option<Accessibility>) {
       *node = None;  // Just sets to None without calling hooks
   }

   This bypasses the hook system entirely - is this intentional?

🚀 Performance Considerations

1. Allocation Overhead: FxHashMap and FxHashSet in TypeScriptCtx are good choices for performance.

2. Hook Chain Overhead: The OrderedChain pattern creates nested function calls. For deeply nested hook chains, this could impact stack usage. The code uses maybe_grow_default in some places but not consistently.

3. Clone in enter_program: Line 36 in transform.rs:

   export_name: ctx.transform.exported_binding.clone(),

   This clones the entire exported bindings map. For large codebases, consider using Arc or references.

📝 Code Quality

Positive:

• Follows repository conventions (from AGENTS.md)
• Good use of Rust idioms (Option, FxHashMap)
• Macro usage for reducing boilerplate in hook_utils.rs is clean

Needs Improvement:

• Documentation: TypeScriptCtx fields lack doc comments explaining their purpose
• Error Handling: No error propagation mechanism in hooks (uses HANDLER from swc_common)
• Testing: Unit tests for individual hooks are absent

🔒 Security

No significant security concerns. The code doesn't handle untrusted input directly (parsing is upstream).

📋 Recommendations

1. Block merging until test failures are fixed. 18% failure rate is too high for a refactor.

2. Consolidate type stripping logic into StripTypeHook only.

3. Add extensive comments to RefRewriter state management logic.

4. Add unit tests for each hook individually:

   #[test]
   fn test_strip_type_hook_removes_type_annotations() { ... }

5. Consider adding integration tests comparing old vs new output for the same inputs.

6. Document the hook execution order in hooks/mod.rs with examples.

7. Add Debug implementations to TypeScriptCtx and state structs for debugging.

🎯 Verdict

NEEDS WORK before merging. The architectural direction is excellent and follows the project's design patterns, but:

• 34 test failures must be resolved
• Namespace scoping changes need investigation
• Documentation and testing need strengthening

Once these issues are addressed, this will be a solid improvement to the codebase.

---

Review performed by Claude Code on 2025-12-31

[claude]

Pull Request Review: Refactor TypeScript transforms to use VisitMutHook pattern

Overview

This is a substantial architectural refactoring that converts the TypeScript transform implementation from direct VisitMut to a composable hook-based pattern. The approach follows the established swc_ecma_transformer architecture and represents a significant improvement in code organization. However, there are critical bugs and code quality issues that must be addressed before merging.

---

✅ Strengths

1. Clean Architecture: The hook composition pattern with HookBuilder is well-designed and makes the transform pipeline clear and composable.

2. State Management: Centralizing state in TypeScriptCtx with separate sub-contexts (TransformState, StripImportExportState, StripTypeState) provides good separation of concerns.

3. Backwards Compatibility: The public API (typescript(), strip(), tsx()) is preserved, making this a non-breaking change.

4. Proper Hook Ordering: The hook chain order (StripImportExport → StripType → Transform) is logical and well-documented.

---

🚨 Critical Issues

1. Logic Bug in exit_stmts (transform.rs:320-324)

Location: crates/swc_ecma_transforms_typescript/src/hooks/transform.rs:320-324

fn exit_stmts(&mut self, node: &mut Vec<Stmt>, ctx: &mut TypeScriptCtx) {
    node.retain_mut(|stmt| {
        let is_empty = stmt.is_empty();
        // Remove those folded into Empty
        is_empty || \!stmt.is_empty()  // ❌ ALWAYS TRUE\!
    });

Problem: This condition is a tautology that always evaluates to true:

• If is_empty == true, then true || false = true
• If is_empty == false, then false || true = true
• Result: No statements are ever filtered out

Fix:

node.retain_mut(|stmt| \!stmt.is_empty());

This matches the correct implementation in exit_module_items at line 209-212.

---

2. Missing Hook Methods in VisitMutHook Implementations

Several hooks in swc_ecma_hooks are missing from hook_utils.rs chain implementations:

Missing from OptionalHook and OrderedChain:

• enter_opt_ts_type / exit_opt_ts_type
• enter_opt_ts_type_ann / exit_opt_ts_type_ann
• enter_opt_ts_type_param_decl / exit_opt_ts_type_param_decl
• enter_opt_ts_type_param_instantiation / exit_opt_ts_type_param_instantiation

While these are currently handled in TypeScriptVisitMutWithHook, if a hook needs to intercept these methods, it won't be called. This could lead to subtle bugs if hooks are composed differently in the future.

Recommendation: Add these methods to the macro-generated implementations for completeness and future-proofing.

---

3. Large Amount of Dead Code (38 compiler warnings)

The compiler reports significant unused code:

Never constructed structs:

• StripImportExport
• StripTsImportEquals (actually used, likely a false warning)
• Transform
• EnumMemberItem
• ExportQuery (actually used, visibility issue)
• InitArg

Never used functions:

• transform()
• enum_member_id_atom()
• id_to_var_declarator() (actually used at line 224, 329 - false warning)
• get_enum_id()
• get_member_key()

Never used traits:

• ModuleId

Action Required:

1. Remove genuinely unused code (reduces binary size and maintenance burden)
2. Fix visibility for types that are used but marked as "never constructed" (like ExportQuery)
3. Add #[allow(dead_code)] with explanations if keeping for future use

---

⚠️ Significant Issues

4. Visibility Warnings for Public State Fields

Location: crates/swc_ecma_transforms_typescript/src/hooks/context.rs

warning: type `TsEnumRecordKey` is more private than the item `TransformState::enum_record`
warning: type `TsEnumRecordValue` is more private than the item `TransformState::get_enum_value`
warning: type `UsageCollect` is more private than the item `StripImportExportState::usage_info`
warning: type `DeclareCollect` is more private than the item `StripImportExportState::declare_info`

Problem: Public struct fields use private types. This is technically allowed but can cause API issues.

Fix Options:

1. Make the types public if they're part of the API surface
2. Make the struct fields pub(crate) if they shouldn't be public
3. Add getter methods instead of direct field access

---

5. Missing Documentation

The new architecture lacks comprehensive documentation:

Module documentation missing:

• hooks/mod.rs - should explain the hook pattern, execution order, and how to add new hooks
• hooks/context.rs - should document the state management strategy
• Individual hook files should explain their responsibilities

Struct documentation needed:

• TypeScriptCtx and all state structs need doc comments
• TypeScriptVisitMutWithHook needs to explain the noop_visit_mut_type\! pattern
• Hook methods should document when they run (enter vs exit) and what they do

Recommendation: Add comprehensive module-level and type-level documentation before merging, especially since this is a new pattern in the codebase.

---

6. Unused Imports

Several files have unused imports:

warning: unused import: `VisitMut`
warning: unused import: `rustc_hash::FxHashSet`
warning: unused import: `ident::IdentLike`

Action: Run cargo fix --lib -p swc_ecma_transforms_typescript and commit the changes.

---

💡 Code Quality Suggestions

7. Inconsistent Error Handling in RefRewriter Operations

Location: context.rs:106-131 (ExportQuery implementation)

The QueryRef implementation uses .clone() on Id values extensively. While this works, consider:

• Are these clones necessary? Id is relatively cheap to clone, but reducing allocations helps performance.
• The `?" early returns could be made more explicit with comments explaining when they occur.

---

8. Complex State Management in transform.rs

Location: hooks/transform.rs

The TransformState has many temporary state fields:

• ref_rewriter_temp
• ref_rewriter_saved_for_var_decl
• saved_namespace_id
• collector_saved_namespace_id
• saved_var_list

Concern: This state management is complex and error-prone. Consider:

1. Adding assertions to verify state consistency
2. Documenting the lifecycle of each temporary field
3. Using RAII-style guards to automatically save/restore state

Example:

struct NamespaceGuard<'a> {
    state: &'a mut TransformState,
    saved: Option<Id>,
}

impl<'a> Drop for NamespaceGuard<'a> {
    fn drop(&mut self) {
        self.state.namespace_id = self.saved.take();
    }
}

---

9. Macro Repetition in hook_utils.rs

The optional_method\! and chained_method\! macros are nearly identical. Consider:

• Unifying them into a single macro with a mode parameter
• Or use a code generation tool if more hooks are added

This is a minor issue but improves maintainability.

---

📊 Test Coverage

Test Status

According to the PR description: 156/190 tests passing (82%)

Concern: 18% test failure rate is quite high for a refactoring PR. While the PR description mentions "edge cases that will be addressed in follow-up commits," this is risky:

1. Risk of breakage: Merging with known test failures can introduce regressions
2. Hard to review: Without passing tests, it's difficult to verify correctness
3. Follow-up complexity: Fixing tests after merging is harder than fixing them before

Strong Recommendation:

• Fix all test failures before merging, OR
• Clearly document which tests are failing and why they're acceptable to defer
• Add TODO comments in code for any known issues

---

🔒 Security Considerations

No security concerns identified. The refactoring:

• Doesn't change the transform logic significantly
• Properly handles TypeScript-specific constructs
• Maintains separation between type-level and value-level code

---

🎯 Performance Considerations

Positive:

• Hook composition is zero-cost at runtime (all inline-able)
• State is properly allocated once and reused

Potential Concerns:

1. Multiple passes over AST: The Collector visitor in enter_program adds an extra pass. Measure if this impacts performance on large files.
2. State cloning: enum_record and exported_binding are cloned when creating RefRewriter. Consider using Rc or references if this becomes a bottleneck.

Recommendation: Run benchmarks comparing old vs new implementation on large TypeScript files.

---

📋 Action Items Summary

Must Fix Before Merge (Critical):

1. ✅ Fix the tautology bug in exit_stmts (line 323)
2. ✅ Resolve all 38 compiler warnings (remove dead code or fix visibility)
3. ✅ Fix all 34 failing tests or document why they're deferred
4. ✅ Add visibility attributes or make private types public

Should Fix Before Merge (Important):

5. ⚠️ Add module-level and struct-level documentation
6. ⚠️ Add missing hook methods to composition macros
7. ⚠️ Run cargo fmt and cargo fix

Nice to Have (Follow-up):

8. 💭 Add RAII guards for complex state management
9. 💭 Benchmark performance impact of extra AST pass
10. 💭 Consolidate repetitive macros in hook_utils.rs

---

Final Recommendation

🔴 Request Changes

This PR shows excellent architectural thinking and moves the codebase in the right direction. However, the critical logic bug and high test failure rate make it unsafe to merge in its current state. Once the critical issues are addressed and tests are passing, this will be a strong improvement to the codebase.

Estimated effort to fix: The critical issues should take ~2-4 hours to address. Documentation could take another 2-3 hours depending on depth.

---

Positive Notes

Despite the issues, this PR demonstrates:

• ✨ Strong understanding of SWC's visitor pattern
• ✨ Good architectural sense with the hook composition pattern
• ✨ Attention to backwards compatibility
• ✨ Clear commit messages and PR description

Great work on tackling this complex refactoring! 🎉

[claude]

Code Review: PR #11408 - Refactor TypeScript transforms to use VisitMutHook pattern

Overview

This is an excellent architectural refactoring that modernizes the TypeScript transformer to use the VisitMutHook pattern. The PR correctly follows the guidelines in crates/swc_ecma_transformer/AGENTS.md.

Changes: 236 files, +4,239/-827 lines
Performance Impact: +4.95% improvement (372 µs → 354.5 µs)

---

✅ Positive Aspects

1. Excellent Architecture - Clean separation of concerns with composable hook pattern
2. Follows Guidelines - Correctly implements VisitMutHook pattern as specified
3. Performance Improvement - 4.95% faster according to CodSpeed benchmarks
4. Maintainability - Hook composition makes future changes much easier
5. Backward Compatibility - Public APIs remain unchanged
6. Type Safety - Good use of Rust's type system

---

🔴 Critical Issues

1. Logic Bug in exit_module_items (transform.rs:134-138)

// Current (INCORRECT)
node.retain_mut(|item| {
    let is_empty = item.as_stmt().map(Stmt::is_empty).unwrap_or(false);
    is_empty || \!item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
});
// This always evaluates to true and never filters anything

Fix:

node.retain_mut(|item| {
    \!item.as_stmt().map(Stmt::is_empty).unwrap_or(false)
});

---

🟡 Code Quality Issues

1. Missing Documentation

• No module-level documentation explaining the hook architecture
• TypeScriptCtx needs comprehensive documentation
• Hook execution order and dependencies should be documented

2. Duplicate Type Annotation Stripping

Type annotations are being stripped in both:

• TypeScriptVisitMutWithHook
• StripTypeHook

This should be centralized in one location to avoid redundancy.

3. Performance Optimizations (Minor)

Consider these follow-up improvements:

• Use Arc<Config> instead of cloning Config fields in context initialization
• Avoid cloning exported_binding HashMap in hot paths
• The double traversal (collection + transform) might be optimizable

4. Dead Code

Remove unused structs and functions that trigger compiler warnings.

---

📋 Recommendations

Before Merge (Required):

• Fix the exit_module_items logic bug
• Ensure all tests pass consistently
• Remove dead code
• Add basic module-level documentation
• Resolve duplicate type annotation stripping

Follow-up (Nice to Have):

• Document hook execution order and dependencies
• Apply performance optimizations mentioned above
• Add comprehensive usage examples

---

Security & Testing

✅ Security: No security concerns identified
⚠️ Testing: Ensure test pass rate is >95% before merge

---

Overall Assessment

Status: 🟡 In Progress - Not Ready for Merge (due to critical logic bug)

This is impressive architectural work that will significantly improve maintainability. The composable hook pattern is well-designed and the performance improvement is a great bonus. However, the critical logic bug in exit_module_items must be fixed before merge.

Once the critical issue is addressed and tests pass consistently, this will be an excellent addition to the codebase.

Estimated effort to fix critical issues: 1-2 hours

[Copilot]

Pull request overview

This PR refactors the TypeScript transforms to use the VisitMutHook pattern, moving from a traditional VisitMut implementation to a composable hook-based architecture. The refactoring improves code organization by separating concerns into distinct hooks (StripImportExport, StripType, Transform) and using a context object to share state.

Key Changes:

• Refactored main transform logic from transform.rs into a hook-based pattern in hooks/transform.rs
• Introduced TypeScriptCtx to centralize state management
• Created composable hooks for different transformation phases
• Updated test snapshots to reflect minor variable naming changes due to scoping differences

Reviewed changes

Copilot reviewed 236 out of 237 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
typescript.rs	Refactored to use hook-based pattern with TypeScriptVisitMutWithHook wrapper
transform.rs	Removed - logic moved to hooks/transform.rs
hooks/mod.rs	New module exposing typescript_hook factory function
hooks/context.rs	New context struct for sharing state across hooks
hooks/transform.rs	Transform logic moved from old transform.rs
hooks/strip_type.rs	StripType hook implementation
hooks/strip_import_export.rs	StripImportExport hook implementation
hooks/typescript_visitor.rs	Visitor wrapper implementing VisitMut with hook integration
strip_import_export.rs	Made methods public for use by hooks
lib.rs	Added hooks module export
Test snapshots	Variable scoping changes (e.g., M → _$M) due to refactoring

---

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

[claude]

PR Review: TypeScript Transformer VisitMutHook Refactoring

Executive Summary

✅ Recommendation: APPROVE

This PR successfully refactors the TypeScript transformer from a direct VisitMut implementation to use the VisitMutHook pattern. The refactoring follows architectural guidelines in crates/swc_ecma_transformer/AGENTS.md and fixes several bugs in namespace handling while maintaining backwards compatibility.

Changes: 237 files, +4,230/-2,619 lines (primarily test output updates)

---

Architecture Analysis

✅ Adherence to AGENTS.md Guidelines

The refactoring correctly follows all three requirements:

1. Transformer implements VisitMut ✓

   • TypeScriptVisitMutWithHook<H> implements VisitMut and delegates to hooks

2. Subtypes don't implement VisitMut ✓

   • All hooks (StripTypeHook, StripImportExportHook, TransformHook) implement VisitMutHook<TypeScriptCtx> instead

3. Subtypes implement VisitMutHook ✓

   • All hooks properly implement the hook pattern with TypeScriptCtx as context

Hook Composition

pub fn typescript_hook(config: Config) -> impl VisitMutHook<TypeScriptCtx> {
    let hook = HookBuilder::new(NoopHook)
        .chain_optional(/* StripImportExport */)
        .chain(strip_type::hook())
        .chain(transform::hook())
        .build()
}

Strengths:

• Clear separation of concerns into three focused hooks
• Correct execution order (import/export → type stripping → transformation)
• Conditional hook inclusion based on configuration

---

Code Quality Assessment

✅ Strengths

1. Type Safety: Proper handling of TypeScript-only nodes prevents invalid traversals
2. Scope Management: Stack-based var_list handling correctly manages nested scopes
3. State Centralization: TypeScriptCtx provides clear ownership and state isolation
4. Bug Fixes: Namespace reference bugs fixed (visible in test outputs)
5. Performance: One-pass transformation with single upfront collection phase

⚠️ Areas for Improvement

1. Complex Namespace State Management

Location: crates/swc_ecma_transforms_typescript/src/hooks/transform.rs

The namespace state uses multiple temporary fields:

• namespace_id / saved_namespace_id
• ref_rewriter / ref_rewriter_temp / ref_rewriter_saved_for_var_decl

Recommendation: Consider using explicit stacks for both to simplify logic and reduce duplication between TsNamespaceDecl and TsModuleDecl handling.

2. Missing Documentation

Location: crates/swc_ecma_transforms_typescript/src/hooks/context.rs

The TransformState struct has 15+ fields with minimal documentation.

Recommendation: Add doc comments explaining the purpose of each field, especially temporary storage fields.

3. Logic Duplication

Namespace handling is duplicated between enter_ts_namespace_decl/exit_ts_namespace_decl and enter_ts_module_decl/exit_ts_module_decl.

Recommendation: Extract common logic into shared helper methods.

---

Test Output Analysis

Improvements in Generated Code

Example from TwoInternalModulesThatMergeEachWithExportedModulesOfTheSameName.1.normal.js:

Before (buggy):

(function(A) {
    (function(B) {})(B || (B = {}));  // BUG: should be A.B
    var B;
})(A || (A = {}));

After (correct):

(function(A) {
    var B;
})(A || (A = {}));
export var Y;

Patterns Fixed

1. ✅ Empty namespace IIFEs properly removed
2. ✅ Variable hoisting now correct (var X, Y, Z;)
3. ✅ Export declarations properly generated (export var Y;)
4. ✅ Nested namespace references fixed
5. ✅ Const enum inlining works correctly

---

Performance Considerations

1. Collection Phase: Adds one extra AST traversal but necessary for:

   • Forward enum references
   • Export detection
   • Building enum value record

   Verdict: Acceptable trade-off for correctness ✓

2. RefRewriter Creation: Frequent creation during namespace traversal

   Verdict: Not a concern unless profiling shows bottleneck ✓

3. Atom Handling: Correctly uses Cow<str> and &str per guidelines ✓

---

Security & Correctness

✅ No security concerns identified
✅ Proper scope handling prevents variable leaks
✅ Const enum inlining respects is_lhs to avoid incorrect transformations
✅ Ambient modules (quoted names) correctly skipped

---

Checklist Against AGENTS.md

• ✅ Write performant code
• ✅ Write comments in English
• ✅ No unstable/nightly features
• ✅ Prefer Cow<str>/&str for Atoms
• ⚠️ Write unit tests (integration tests updated, could add unit tests)
• ✅ Don't remove/modify existing tests
• ⚠️ Write documentation (could be improved)
• ⚠️ Run cargo fmt --all (should verify before merge)
• ✅ Commit changes

---

Final Recommendations

Before Merge

1. ⚠️ Run cargo fmt --all to ensure formatting compliance
2. ⚠️ Consider adding doc comments to TransformState fields

Future Improvements (Not Blocking)

1. Extract common namespace handling logic
2. Simplify state management with explicit stacks
3. Add unit tests for hook composition
4. Add debug assertions for state validation

---

Conclusion

Overall Assessment: EXCELLENT ✅

This is a high-quality refactoring that:

• Follows architectural guidelines correctly
• Improves code maintainability through clear separation of concerns
• Fixes existing bugs in namespace handling
• Maintains comprehensive test coverage
• Shows actual improvements in generated code quality

Confidence Level: HIGH

The extensive test updates (237 files) demonstrate thorough validation of the changes. The architectural pattern aligns with existing transformers in the codebase.

Great work on this refactoring! 🎉

[claude]

Code Review: Refactor TypeScript Transform to VisitMutHook Pattern

This is a substantial architectural refactoring that migrates the TypeScript transform from a monolithic VisitMut implementation to a composable VisitMutHook pattern. Overall, this is a well-executed refactoring that improves modularity and maintainability. However, I've identified some areas that warrant attention.

---

✅ Strengths

1. Clean Separation of Concerns

The refactoring successfully separates the TypeScript transformation into three distinct, composable hooks:

• StripImportExportHook - Removes unused imports/exports
• StripTypeHook - Strips TypeScript-only syntax
• TransformHook - Transforms TypeScript constructs to JavaScript

This modular design makes the codebase much easier to understand and maintain.

2. Centralized State Management

The new TypeScriptCtx structure (context.rs:13-27) consolidates all transformation state in one place with clear ownership boundaries. Each hook gets its own state struct (TransformState, StripImportExportState, StripTypeState), which prevents state leakage between concerns.

3. Flexible Composition

The HookBuilder pattern (hook_utils.rs:406-442) provides a clean, fluent API for composing hooks:

let hook = HookBuilder::new(NoopHook)
    .chain_optional(strip_import_export)  // Conditional based on config
    .chain(strip_type)
    .chain(transform)
    .build()

This makes it easy to conditionally enable/disable transformation passes.

4. Adherence to AGENTS.md Guidelines

✅ Implements VisitMutHook pattern as specified in crates/swc_ecma_transformer/AGENTS.md
✅ Subtypes implement VisitMutHook<TraverseCtx> (here: TypeScriptCtx)
✅ Main visitor executes VisitMutHooks of its subtypes

---

⚠️ Issues & Recommendations

1. Complex RefRewriter State Management 🔴 High Priority

Location: context.rs:61-63, transform.rs:89-122, 399-416

The TransformState maintains three separate RefRewriter fields:

pub(crate) ref_rewriter: Option<RefRewriter<ExportQuery>>,
pub(crate) ref_rewriter_temp: Option<RefRewriter<ExportQuery>>,
pub(crate) ref_rewriter_saved_for_var_decl: Option<RefRewriter<ExportQuery>>,

This leads to complex save/restore logic across multiple hooks:

• ref_rewriter_temp: Saved when entering nested namespaces
• ref_rewriter_saved_for_var_decl: Saved for non-export variable declarators

Risk: If any exit hook fails to properly restore the saved state, the rewriter could be in an incorrect state for subsequent nodes.

Recommendation:

• Add debug assertions in exit hooks to verify state invariants
• Consider a stack-based approach for managing nested contexts
• Document the exact lifetime and restoration guarantees for each field

2. Type Annotation Removal Split 🟡 Medium Priority

Location: typescript_visitor.rs:259-335

Type annotation removal is split between two places:

1. Visitor wrapper sets type annotations to None before visiting children
2. StripType hook removes other TypeScript-specific syntax

fn visit_mut_class_prop(&mut self, node: &mut ClassProp) {
    node.type_ann = None;  // Done in visitor wrapper
    node.visit_mut_children_with(self);
    self.hook.exit_class_prop(node, &mut self.context);
}

Why this exists: The noop_visit_mut_type\!() macro prevents traversal into type nodes, but some nodes have non-optional type fields that would still be visited.

Recommendation:

• Add a comment in StripTypeHook explaining why it doesn't handle these cases
• Document this design decision in the module-level docs for typescript_visitor.rs

3. is_lhs Tracking Limitation 🟡 Medium Priority

Location: typescript_visitor.rs:206-222, transform.rs:456-462

The is_lhs flag tracking has an acknowledged limitation for nested member expressions:

// We'll need to restore it, but VisitMutWithHook doesn't give us
// per-node state. So we'll just set it to false and it will be
// corrected by parent

Risk: This could lead to incorrect transformation of deeply nested member expressions in LHS position.

Recommendation:

• Add test cases for nested member expressions in assignment positions
• Consider using a stack-based approach for LHS tracking
• Document the known limitation if it's an acceptable trade-off

4. Hook Ordering Not Enforced at Type Level 🟡 Medium Priority

Location: mod.rs:19-32

The correct ordering of hooks is documented in comments but not enforced:

// Chain hooks in the correct order:
// 1. StripImportExport (optional)
// 2. StripType
// 3. Transform

Risk: If someone creates a different composition (e.g., for testing), incorrect ordering could cause subtle bugs.

Recommendation:

• Make typescript_hook() the only public API (it appears to already be the case)
• Consider using a builder pattern that enforces ordering at compile time
• Add a module-level doc comment explaining why ordering matters

5. Performance Consideration: Multiple Empty Statement Filters

Location: transform.rs:319-324, 209-212; strip_type.rs:305-307

All three hooks filter empty statements, which means the same filtering logic runs multiple times:

// In exit_stmts:
stmts.retain(|stmt| \!matches\!(stmt, Stmt::Empty(..)));

Impact: Minor - filtering is O(n) but cheap. However, this could be optimized.

Recommendation:

• Profile to see if this has measurable impact
• If optimizing, consolidate filtering into a final cleanup pass

---

🔍 Positive Observations

1. No TODOs/FIXMEs: Clean code with no technical debt markers
2. Consistent Error Handling: Uses HANDLER.err() consistently for diagnostics
3. Good Use of maybe_grow_default: Prevents stack overflows on deeply nested ASTs (strip_type.rs:13)
4. Proper Span Handling: Maintains source spans throughout transformations

---

📋 Testing Recommendations

The refactoring maintains behavioral compatibility, but I recommend:

1. Test nested namespace exports - verify RefRewriter state management
2. Test nested member expressions in LHS - check is_lhs tracking limitation
3. Test hook ordering assumptions - ensure StripImportExport → StripType → Transform order
4. Test conditional hook disabling - verify behavior when verbatim_module_syntax: true
5. Benchmark performance - ensure no regression from the refactoring

---

🎯 Overall Assessment

This is a well-designed refactoring that successfully modernizes the TypeScript transform to use the VisitMutHook pattern. The identified issues are mostly edge cases and complexity inherent to TypeScript transformation rather than flaws in the refactoring approach.

The most significant concern is the RefRewriter state management complexity, which would benefit from additional documentation and safeguards.

Recommendation: Approve with suggestions for follow-up improvements.

Great work on this substantial architectural improvement! 🚀

[claude]

Code Review: TypeScript VisitMutHook Refactoring

This is a substantial architectural refactoring that converts the TypeScript transform from a monolithic VisitMut implementation to a composable VisitMutHook pattern. Overall, this is a high-quality refactoring that improves code organization and maintainability. Here's my detailed feedback:

---

✅ Strengths

Architecture & Design

• Excellent separation of concerns: The code is now split into clear modules (strip_type, strip_import_export, transform) with well-defined responsibilities
• Composable hook pattern: The use of HookBuilder and OrderedChain allows flexible composition of transformation passes
• Centralized state management: TypeScriptCtx consolidates all state, making it easier to reason about data flow
• Backwards compatibility: Public API (typescript(), strip(), tsx()) remains unchanged

Code Quality

• Comprehensive commit history: The 27 commits show incremental progress from 82% → 100% test pass rate, demonstrating careful iteration
• Well-documented state: Clear comments explain tricky parts (e.g., RefRewriter scoping, var_list stack management)
• Performance-conscious: Uses maybe_grow_default wrapper to prevent stack overflow on deeply nested expressions

---

🔍 Issues & Concerns

1. Critical: Stack Overflow Prevention Incomplete ⚠️

// typescript_visitor.rs:413
fn visit_mut_expr(&mut self, n: &mut Expr) {
    maybe_grow_default(|| {
        // only call hooks, don't visit children
        self.hooks.enter_expr(n, &mut self.ctx);
        self.hooks.exit_expr(n, &mut self.ctx);
    });
}

Problem: You're wrapping the hook calls but not the recursive child visit. The stack overflow protection only applies to hook execution, but deeply nested expressions will still overflow when children are visited via other methods.

Fix: Either wrap the entire visit (including children) or ensure all recursive visits use maybe_grow_default.

---

2. Redundant #[cfg(swc_ast_unknown)] Guards

Multiple locations have this pattern:

#[cfg(swc_ast_unknown)]
_ => panic\!("unable to access unknown nodes"),

Issue: These catch-all arms add noise without clear benefit. If a new AST variant is added, the compiler will warn about non-exhaustive matches anyway.

Recommendation: Remove these unless there's a specific requirement to panic on unknown variants.

---

3. Complex RefRewriter State Management

The RefRewriter save/restore logic is error-prone:

// transform.rs:68-69
ctx.transform.ref_rewriter_temp = ctx.transform.ref_rewriter.take();
// ... later restored in fold_decl

Issues:

• Temporal coupling: ref_rewriter_temp must be set in enter_* and cleared in fold_decl (not exit_*)
• No validation: If restoration is missed, state leaks silently
• Three RefRewriter fields: ref_rewriter, ref_rewriter_temp, ref_rewriter_saved_for_var_decl is confusing

Suggestions:

• Use a stack-based approach (like saved_var_list) for consistency
• Add debug assertions to validate proper restoration
• Document why restoration must happen in fold_decl vs exit_*

---

4. Potential Bug: is_lhs Tracking

// transform.rs:1183-1191
fn enter_assign_expr(&mut self, n: &mut AssignExpr, ctx: &mut TypeScriptCtx) {
    let saved = ctx.transform.is_lhs;
    ctx.transform.is_lhs = true;
    // visit left side
    ctx.transform.is_lhs = false;
    // visit right side
    ctx.transform.is_lhs = saved;
}

Problem: This pattern is fragile. If an exception occurs or early return happens between setting is_lhs = true and restoration, state can be corrupted.

Recommendation: Use RAII pattern with a guard struct:

struct IsLhsGuard<'a> { ctx: &'a mut TypeScriptCtx, saved: bool }
impl Drop for IsLhsGuard<'_> {
    fn drop(&mut self) { self.ctx.transform.is_lhs = self.saved; }
}

---

5. Silent Filtering Without Warnings

// strip_import_export.rs:86-100
specifiers.retain(|export_specifier| match export_specifier { ... });

Issue: When exports are filtered due to being type-only, there's no diagnostic. Users may be confused why exports disappear.

Recommendation: Consider adding trace-level logging when significant transformations occur, especially for debugging.

---

6. Inconsistent Error Handling

Some code paths use unreachable\!(), others use panic\!(), and some silently return None.

Example:

// strip_type.rs:125
Decl::TsModule(..) => unreachable\!("use exit_module_item"),

Recommendation: Establish consistent error handling conventions:

• Use unreachable\!() for genuinely impossible states
• Use debug_assert\! for performance-critical invariants
• Provide context in error messages

---

7. Code Duplication in Hook Wrappers

hook_utils.rs has extensive macro-generated code for OptionalHook and OrderedChain:

optional_method\!(enter_expr, exit_expr, Expr);
optional_method\!(enter_stmt, exit_stmt, Stmt);
// ... 40+ more

Observations:

• This is 442 lines of mostly boilerplate
• Maintainability burden: Adding new hook methods requires updating multiple macros

Suggestion: Consider if blanket trait implementations or procedural macros could reduce this boilerplate.

---

📊 Performance Considerations

Positive

• Single-pass architecture: Hooks run in coordinated passes rather than multiple separate traversals
• Lazy initialization: RefRewriters only created when needed
• Stack-based scoping: saved_var_list uses efficient Vec stack

Potential Concerns

1. Enum value collection happens twice: Once in Collector visitor in enter_program, then checked in enter_ts_enum_decl. Consider removing the second computation (commit 6ebc06e addresses this)
2. Multiple hash map lookups: Consider caching frequently accessed exported_binding lookups
3. Cloning in RefRewriter: Lines 72-83 in transform.rs clone entire export maps for nested namespaces

---

🔒 Security Considerations

✅ No security concerns identified. The transformation correctly:

• Strips types without exposing private type information
• Maintains proper scoping for namespace exports
• Handles ambient declarations safely

---

🧪 Test Coverage

Excellent test progression:

• Initial commit: 156/190 (82%)
• Final state: 190/190 (100%) + 5036/5036 correctness tests

Recommendation: Add regression tests for the edge cases fixed in commits 18-27:

• Nested namespace RefRewriter scoping
• is_lhs tracking in assignments
• var_list scoping in nested blocks
• Ambient module handling

---

📝 Documentation Needs

1. Add module-level docs explaining the hook architecture and execution order
2. Document state lifecycle for complex fields like ref_rewriter, var_list, namespace_id
3. Explain why restoration happens in fold_decl rather than exit_* hooks
4. Add examples of how to add new hooks to the chain

---

🎯 Recommendations by Priority

High Priority (Before Merge)

1. ✅ Fix stack overflow prevention in visit_mut_expr (wrap entire visit)
2. ✅ Add debug assertions for RefRewriter restoration invariants
3. ✅ Document RefRewriter temporal coupling in comments

Medium Priority (Follow-up PRs)

4. Refactor RefRewriter to use stack-based pattern
5. Add RAII guards for is_lhs state management
6. Add module-level documentation

Low Priority (Future Improvements)

7. Reduce boilerplate in hook_utils.rs
8. Add trace logging for debugging transforms
9. Benchmark memory allocation patterns

---

✨ Conclusion

This is a well-executed refactoring that successfully achieves its architectural goals. The progression from 82% to 100% test coverage through 27 incremental commits demonstrates careful engineering. The code follows SWC conventions and maintains backwards compatibility.

Recommendation: Approve with minor fixes to the stack overflow handling and RefRewriter documentation.

Great work on tackling this complex refactoring! 🎉

---

Review conducted following CLAUDE.md conventions: performance-focused, well-documented, with unit tests and proper formatting.

[claude]

Pull Request Review: Refactor TypeScript Transforms to VisitMutHook Pattern

Overview

This is a substantial architectural refactoring that migrates the TypeScript transform implementation from direct VisitMut trait implementations to the composable VisitMutHook pattern. This aligns with the architecture established in swc_ecma_transformer and represents a significant improvement in code organization.

✅ Strengths

1. Architecture & Design

• Composable Hook Pattern: The refactoring successfully adopts a modular, composable architecture with separate hooks for different concerns (StripTypeHook, StripImportExportHook, TransformHook)
• Single VisitMut Implementation: Moving to a single TypeScriptVisitMutWithHook that handles all TypeScript nodes is cleaner than having multiple VisitMut implementations
• Clear Separation of Concerns: Each hook has a well-defined responsibility, making the codebase more maintainable
• Centralized State Management: The TypeScriptCtx struct cleanly organizes all transform state in one place

2. Code Quality

• Good Documentation: The context structs and hook functions have helpful comments explaining their purpose
• Type Safety: Proper use of Rust's type system with clear trait bounds
• Backwards Compatibility: The public API (typescript(), strip(), tsx()) remains unchanged, which is excellent for consumers

3. Implementation Details

• Hook Composition: The HookBuilder pattern with OrderedChain and OptionalHook is elegant
• TypeScript Node Handling: The special handling in TypeScriptVisitMutWithHook to prevent visiting type annotations is correct (lines 35-61 in typescript_visitor.rs)
• Performance Considerations: Use of maybe_grow_default() for deep recursion (typescript_visitor.rs:103)

⚠️ Concerns & Issues

1. Code Duplication (Medium Priority)

Location: hooks/context.rs:35-38 and hooks/context.rs:54-57

// In TypeScriptCtx::new()
import_export_assign_config: config.import_export_assign_config,
ts_enum_is_mutable: config.ts_enum_is_mutable,
verbatim_module_syntax: config.verbatim_module_syntax,
native_class_properties: config.native_class_properties,

These config fields are duplicated between TypeScriptCtx.config and TransformState. This could lead to inconsistencies if not carefully maintained.

Recommendation: Consider having TransformState borrow/reference these from config instead of duplicating them, or document why duplication is necessary.

2. Missing Clone Implementation (Low Priority)

Location: hooks/context.rs:35

The config is moved into the context on line 46, but fields are also copied into TransformState. If Config doesn't implement Copy, this could cause issues.

Recommendation: Verify that Config implements Clone and the clone is intentional, or adjust to use references.

3. Inconsistent Access Modifiers

Location: Throughout hooks/context.rs

Some fields in TransformState are pub(crate) while the struct itself is public. The visibility strategy seems inconsistent.

Recommendation: Document the visibility design - are these internal implementation details that shouldn't be accessed directly?

4. Clone on Every Access (Performance)

Location: hooks/context.rs:106-109

self.export_name
    .get(&export_name.to_id())?
    .clone()  // Unnecessary clone of Option<Id>
    .map(|namespace_id| ...)

The .clone() followed by .map() on an Option<Id> is inefficient. The Option itself doesn't need to be cloned.

Recommendation:

self.export_name
    .get(&export_name.to_id())
    .and_then(|opt_id| opt_id.as_ref())
    .map(|namespace_id| ...)

This same pattern appears in multiple methods of ExportQuery.

5. Test Coverage Concerns (High Priority)

Commit Message: "Test results: 156/190 tests passing (82%). Remaining failures are edge cases that will be addressed in follow-up commits."

18% test failure rate is concerning for a refactoring PR. While the commit message acknowledges this, having failing tests merged could:

• Hide regressions
• Make bisecting future issues harder
• Break downstream users

Recommendation:

• Document the failing test cases with issue numbers
• Ensure the failures are truly edge cases and not core functionality
• Consider marking this PR as draft until test pass rate is higher (95%+)
• Add a TODO.md or tracking issue listing the known failures

6. Missing Default Implementation (Potential Bug)

Location: hooks/context.rs:52-82

#[derive(Default)]
pub struct TransformState {
    pub(crate) import_export_assign_config: TsImportExportAssignConfig,
    // ...
}

TransformState derives Default, but fields like import_export_assign_config may not have sensible defaults. This could cause issues if Default::default() is called anywhere.

Recommendation: Either remove #[derive(Default)] or document that default construction is only used internally and the defaults are correct for that use case.

7. Incomplete Documentation

While the code has some comments, several areas could benefit from more documentation:

• The lifecycle and ordering of hook execution
• Why certain hooks need to run before others
• What state is shared vs. isolated between hooks
• Performance characteristics of the hook chain

8. TypeScript Type Node Handling (Critical to Verify)

Location: typescript_visitor.rs:35-61

The manual stripping of type annotations in visit_mut_opt_ts_type_ann and similar methods is correct, but it's critical that ALL code paths that could reach TypeScript type nodes are covered.

Recommendation: Add integration tests that specifically verify type nodes are never visited, perhaps with an assertion hook that panics if it encounters one.

🔍 Specific Code Review Comments

hooks/hook_utils.rs

Lines 19-127: The macro-based implementation of OptionalHook and OrderedChain is clever, but generates a lot of boilerplate. Consider if a blanket implementation or proc-macro could reduce this.

hooks/strip_type.rs

Lines 86-104: The two-pass approach to cleaning up class members (filter, then clean flags) is good and necessary. Well commented.

Lines 147-159: The while let loop unwrapping nested type assertions is correct and handles the recursive nature well.

hooks/transform.rs

Lines 62-99: The namespace handling with saved_namespace_id and ref_rewriter_temp is complex. This would greatly benefit from a diagram or more detailed comments explaining the state transitions.

Lines 102-108: Good comment explaining WHY ref_rewriter restoration is delayed. This kind of documentation is valuable.

🎯 Recommendations Summary

Before Merge:

1. Fix or document the 34 failing tests - This is critical
2. Address the performance issue with unnecessary clones in ExportQuery
3. Add documentation explaining the hook execution order and state management
4. Verify type node handling is complete with integration tests

Nice to Have:
5. Consider reducing duplication between config and TransformState fields
6. Add more inline documentation for complex state transitions
7. Consider a follow-up PR to reduce boilerplate in hook_utils.rs

📊 Overall Assessment

This is a well-architected refactoring that moves the codebase in the right direction. The composable hook pattern is superior to the previous monolithic approach. However, the 18% test failure rate is a blocker that must be addressed before merge.

Recommendation: Mark as draft, fix the failing tests, then re-request review.

Compliance with CLAUDE.md

✅ Follows performance-first principles
✅ Code is well-commented in English
✅ No unstable Rust features used
⚠️ Test failures need resolution
✅ Code appears to follow existing patterns

---

Review conducted by Claude Code on behalf of the swc-project team

[kdy1]

I'll open another PR