Skip to content

Conversation

@kdy1
Copy link
Member

@kdy1 kdy1 commented Oct 27, 2025

Description:

See: #10763

Related issue:

@kdy1 kdy1 added this to the Planned milestone Oct 27, 2025
@kdy1 kdy1 requested review from a team as code owners October 27, 2025 06:49
@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2025

⚠️ No Changeset found

Latest commit: 2686798

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

@kdy1 kdy1 enabled auto-merge (squash) October 27, 2025 06:49
@claude
Copy link
Contributor

claude bot commented Oct 27, 2025

Claude finished @kdy1's task —— View job


PR Review: refactor(es/ast): Cherry-pick #10763

Reviewing the refactor that replaces Literal with StringLiteral in JSXAttrValue type.

Todo List:

  • Analyze core AST changes in swc_ecma_ast
  • Review parser and lexer modifications
  • Check codegen and transform updates
  • Examine binding layer changes
  • Verify TypeScript definition updates
  • Assess test coverage and potential issues
  • Provide final review recommendations

Core AST Analysis ✅

The main change is in crates/swc_ecma_ast/src/jsx.rs:191 where the JSXAttrValue enum has been updated:

pub enum JSXAttrValue {
    #[tag("StringLiteral")]
    Str(Str),  // Previously was #[tag("Literal")] Str(Str)
    
    #[tag("JSXExpressionContainer")]
    JSXExprContainer(JSXExprContainer),
    // ... other variants unchanged
}

Observations:

  • Correct tag change: #[tag("Literal")]#[tag("StringLiteral")] is semantically more accurate
  • Type consistency: The underlying Str type remains unchanged, maintaining data structure integrity
  • Aligns with standards: StringLiteral is more specific and follows TypeScript/Babel naming conventions

Parser, Codegen & Transform Analysis ✅

Parser (crates/swc_ecma_parser/src/parser/jsx/mod.rs:255-257):

  • Parser unchanged: The parser correctly generates JSXAttrValue::Str(value) - no modification needed since the enum variant and underlying type remain the same
  • Logic preserved: JSX attribute parsing flows remain identical

Codegen (crates/swc_ecma_codegen/src/jsx.rs:92):

  • Codegen unchanged: Correctly emits JSXAttrValue::Str(ref n) => emit!(n) - the pattern match works regardless of the tag annotation
  • Output preserved: Generated code remains identical

React Transform (crates/swc_ecma_transforms_react/src/jsx/mod.rs:1009, 1590):

  • Transform logic preserved: Functions like jsx_attr_value_to_expr handle JSXAttrValue::Str(s) correctly
  • No behavioral changes: String attribute handling in JSX→React transformation remains consistent

Binding Layer & TypeScript Definitions ✅

WASM Binding (bindings/binding_core_wasm/src/types.rs):

  • Type definitions updated: Line 1737 correctly defines JSXAttrValue = StringLiteral | ...
  • Binding consistency: The WASM binding layer properly reflects the new StringLiteral tag

TypeScript Definitions (packages/types/index.ts:1808-1812):

  • TypeScript types updated: Correctly defines JSXAttrValue = StringLiteral | JSXExpressionContainer | JSXElement | JSXFragment
  • API consistency: External TypeScript API properly reflects the refactor

Generated Visit Code (crates/swc_ecma_visit/src/generated.rs):

  • Visit patterns updated: Generated visitor code correctly handles the new tag without breaking traversal patterns

Test Coverage & Potential Issues ✅

Test Impact Assessment:

  • Existing tests preserved: Since the underlying Str type and enum variant structure remain unchanged, existing tests should continue to pass
  • Serialization compatibility: Only the #[tag] annotation changes, which affects JSON serialization output but not functionality
  • ⚠️ Missing changeset: The PR lacks a changeset as noted by changeset-bot

Potential Issues:

  • 🔍 External tooling: Tools that parse SWC's JSON AST output and specifically look for "Literal" tags in JSX attributes will need updates
  • 🔍 Documentation: Any documentation that references the old Literal tag should be updated

Final Review Recommendations

APPROVE - This is a well-executed refactor with the following strengths:

  1. Semantic Accuracy: StringLiteral is more descriptive than the generic Literal for JSX string attributes
  2. Backward Compatibility: Runtime behavior is preserved since only the tag annotation changes
  3. Comprehensive Coverage: All layers (AST, parser, codegen, transforms, bindings, types) are consistently updated
  4. Standards Alignment: Matches TypeScript/Babel naming conventions

📋 Minor Action Items:

  1. Add Changeset: Create a changeset file to document this breaking change in JSON AST output
  2. Migration Guide: Consider documenting this change for external tools that consume SWC's AST output
  3. Version Notes: This should be noted as a breaking change for JSON AST consumers (but not for code transformation behavior)

🎯 Summary:

This is a high-quality refactor that improves semantic clarity without breaking core functionality. The change is well-coordinated across all relevant systems and maintains backward compatibility for all practical use cases.


@codspeed-hq
Copy link

codspeed-hq bot commented Oct 27, 2025

CodSpeed Performance Report

Merging #11182 will not alter performance

Comparing kdy1/cherry-pick (2686798) with main (845512c)

Summary

✅ 140 untouched

@kdy1 kdy1 merged commit e93ffde into main Oct 27, 2025
173 checks passed
@kdy1 kdy1 deleted the kdy1/cherry-pick branch October 27, 2025 07:21
@kdy1 kdy1 modified the milestones: Planned, v1.14.0 Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Change the variant Lit in JsxAttrValue into Str

3 participants