Skip to content

Branch: SWC v2 #10739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Branch: SWC v2 #10739

wants to merge 7 commits into from

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Jul 1, 2025

No description provided.

Copy link

changeset-bot bot commented Jul 1, 2025

🦋 Changeset detected

Latest commit: 9e887cc

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

@kdy1 kdy1 added this to the v2 (API cleanup) milestone Jul 1, 2025
Copy link

codspeed-hq bot commented Jul 1, 2025

CodSpeed Performance Report

Merging #10739 will not alter performance

Comparing dev/v2 (9e887cc) with main (40a1e2e)

Summary

✅ 140 untouched benchmarks

@magic-akari
Copy link
Member

This branch needs rebase.

@kdy1
Copy link
Member Author

kdy1 commented Jul 23, 2025

Can you do it?

@magic-akari
Copy link
Member

Can you do it?

No. dev/v2 is a protected branch.

@kdy1
Copy link
Member Author

kdy1 commented Jul 23, 2025

After several tries to rebase, I created v2-rebase branch which is not a protected branch.

https://github.com/swc-project/swc/tree/v2-rebase

Can you rebase this branch instead?

@magic-akari
Copy link
Member

magic-akari commented Jul 23, 2025

Changes from #10678 have been merged into main branch, so I'll drop this commit during rebase.
CC @bvanjoi - correct me if I'm wrong.

@bvanjoi
Copy link
Contributor

bvanjoi commented Jul 23, 2025

I think this is ok because this change is not important.

@magic-akari
Copy link
Member

Can you rebase this branch instead?

done

@kdy1
Copy link
Member Author

kdy1 commented Jul 23, 2025

Thank you! I updated dev/v2 branch!

Copy link
Contributor

claude bot commented Jul 23, 2025

Claude finished @kdy1's task —— View job


SWC v2 Pull Request Review

I've completed a comprehensive review of this major version PR focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist

  • Analyze core configuration changes
  • Review dependency updates for security/compatibility
  • Examine JSX/React transform behavioral changes
  • Check for potential performance implications
  • Review test coverage and behavioral changes
  • Assess security concerns
  • Provide comprehensive feedback summary

🚨 Critical Breaking Changes

1. React Transform Default Runtime Change

Impact: HIGH - This affects every JSX transformation by default.

// Before v2 (Classic default)
Runtime::Classic(Default::default())  // Uses React.createElement

// After v2 (Automatic default)  
Runtime::Automatic(Default::default())  // Uses jsx/jsxs from react/jsx-runtime

Migration Required:

{
  "jsc": {
    "transform": {
      "react": {
        "runtime": "classic"  // Add this to maintain v1 behavior
      }
    }
  }
}

2. AST Breaking Change - JSXAttrValue

Impact: MEDIUM - Affects tooling that depends on SWC AST structure.

Change from #[tag("Literal")] to #[tag("StringLiteral")] in JSXAttrValue::Str variant (line 191 in crates/swc_ecma_ast/src/jsx.rs).

3. JSX Transform Processing Order

Impact: MEDIUM - Architectural change in transform pipeline.

Classic JSX transform now runs before the resolver pass (lines 311-334 in config/mod.rs), which could affect custom transforms that depend on scope information.


🔍 Code Quality Assessment

Strengths:

  • Extensive Test Coverage: 100+ test fixtures with comprehensive JSX/React transform test cases
  • Performance Optimization: Benchmarking infrastructure in place (benches/oxc.rs)
  • Backward Compatibility: Explicit runtime configuration still supported
  • TypeScript Preset Support: Added modern preset configurations

Concerns:

  • Error Handling: Some unwrap() calls in config parsing (lines 395, 429, 750 in config/mod.rs)
  • Transform Order Dependency: JSX transform timing changes could introduce subtle bugs
  • Large Changeset: 100 files modified makes thorough review challenging

🔒 Security Assessment

High Priority Issues:

  1. CVE in Dependencies: [email protected] has Critical CVE (GHSA-fjxv-7rqg-78g4) for unsafe random boundary generation
    • Status: Appears in test fixtures, not production code
    • Recommendation: Update to patched version (≥2.5.4)

Medium Priority:

  • Configuration parsing uses panic!() in some error cases (line 1683)
  • Multiple unwrap() calls in configuration validation

⚡ Performance Implications

Positive:

  • Benchmarking infrastructure suggests performance monitoring
  • Automatic runtime may be more efficient than classic for large codebases
  • Caching mechanisms in place for module resolution

Potential Concerns:

  • Transform order changes could affect pipeline efficiency
  • Additional runtime imports for automatic JSX

📋 Recommendations

Before Merging:

  1. Update Dependencies: Address the form-data CVE
  2. Migration Guide: Provide clear documentation for the runtime change
  3. Error Handling: Consider replacing panic!() with proper error propagation
  4. Integration Testing: Verify popular frameworks (Next.js, Vite, etc.) work correctly

For Users:

  1. Test Thoroughly: This is a major version with significant breaking changes
  2. Explicit Configuration: Consider explicitly setting "runtime": "classic" for gradual migration
  3. Bundle Analysis: Check that automatic runtime imports are properly tree-shaken

Code Quality Improvements:

// Instead of:
js_minify.as_ref().unwrap()

// Consider:
js_minify.as_ref().expect("js_minify should be Some at this point")
// or proper error handling

✅ Final Assessment

This is a well-tested major version release with significant but necessary breaking changes. The React transform default change is the most impactful but aligns with modern React practices.

Recommendation: APPROVE with caveats

  • Address the dependency CVE
  • Provide comprehensive migration documentation
  • Consider phased rollout for ecosystem compatibility

The extensive test coverage (100+ fixtures) and performance benchmarking infrastructure demonstrate good engineering practices. The breaking changes are justified for modernizing SWC's default behavior.


Total Files Reviewed: 12 core files + extensive test analysis
Breaking Changes Identified: 3 major
Security Issues: 1 dependency CVE
Test Coverage: Excellent (100+ comprehensive fixtures)

Copy link

socket-security bot commented Jul 23, 2025

Copy link

socket-security bot commented Jul 23, 2025

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert (click for details)
Warn Critical
[email protected] has a Critical CVE.

CVE: GHSA-fjxv-7rqg-78g4 form-data uses unsafe random function in form-data for choosing boundary (CRITICAL)

Affected versions: < 2.5.4; >= 3.0.0 < 3.0.4; >= 4.0.0 < 4.0.4

Patched version: 2.5.4

From: ?npm/[email protected]

ℹ Read more on: This package | This alert | What is a critical CVE?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: Remove or replace dependencies that include known critical CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

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.

3 participants