Skip to content

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

Merged
Donny/강동윤 (kdy1) merged 3 commits into
mainfrom
kdy1/cherry-pick
Oct 27, 2025
Merged

refactor(es/ast): Cherry-pick #10763#11182
Donny/강동윤 (kdy1) merged 3 commits into
mainfrom
kdy1/cherry-pick

Conversation

@kdy1

Copy link
Copy Markdown
Member

Description:

See: #10763

Related issue:

@kdy1 Donny/강동윤 (kdy1) added this to the Planned milestone Oct 27, 2025
@changeset-bot

changeset-bot Bot commented Oct 27, 2025

Copy link
Copy Markdown

⚠️ 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
Donny/강동윤 (kdy1) enabled auto-merge (squash) October 27, 2025 06:49
@claude

claude Bot commented Oct 27, 2025

Copy link
Copy Markdown
Contributor

Claude finished Donny/강동윤 (@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

codspeed-hq Bot commented Oct 27, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #11182 will not alter performance

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

Summary

✅ 140 untouched

@kdy1
Donny/강동윤 (kdy1) merged commit e93ffde into main Oct 27, 2025
173 checks passed
@kdy1
Donny/강동윤 (kdy1) deleted the kdy1/cherry-pick branch October 27, 2025 07:21
@swc-project swc (swc-project) locked as resolved and limited conversation to collaborators Nov 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Change the variant Lit in JsxAttrValue into Str

2 participants