feat(feature-flags): Add two-phase mode support (mark & shake)

[kdy1]

Summary

Add support for two-phase feature flag transformation with mark and shake modes.

Changes

New Mode System

• Mark mode (default): Marks flags with __SWC_FLAGS__ markers for later substitution
• Shake mode: Substitutes markers with boolean values and performs DCE

This provides a clearer two-phase workflow:

1. Phase 1 (mark): Convert flag variables to markers
2. Phase 2 (shake): Substitute markers and eliminate dead code

Implementation

• Add TransformMode enum with Mark and Shake variants
• Add unified FeatureFlagsConfig supporting both modes
• Update plugin to dispatch to BuildTimeTransform (mark) or RuntimeTransform (shake)
• Mark mode is now the default (previously was implicit build-time behavior)

TypeScript Types

• Updated types.d.ts with new TransformMode type
• Added comprehensive JSDoc documentation with examples
• Maintained backward compatibility

Tests

• Added comprehensive WASM integration tests (__tests__/wasm.test.ts)
• Tests for mark mode: marker generation, custom marker objects
• Tests for shake mode: DCE on markers, logical operations
• Tests for edge cases: exclude flags, nested scopes, multiple libraries
• All 8 tests passing ✅

Documentation

• Updated README with two-phase workflow explanation
• Added complete examples showing both phases
• Clarified when to use each mode

Example Usage

Mark Mode (Phase 1)

{
  "mode": "mark",
  "libraries": {
    "@your/flags": {
      "functions": ["useFlags"]
    }
  }
}

Shake Mode (Phase 2)

{
  "mode": "shake",
  "flagValues": {
    "featureA": true,
    "featureB": false
  }
}

Breaking Changes

None - the plugin maintains backward compatibility with the old BuildTimeConfig format.

Test Results

• ✅ Rust crate tests: 5 passed
• ✅ WASM integration tests: 8 passed
• ✅ Build successful

🤖 Generated with Claude Code

[changeset-bot]

⚠️ No Changeset found

Latest commit: db832df

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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]

Code Review: Two-Phase Feature Flag Transformation

Overall Assessment

This is a well-designed PR that adds two-phase mode support to the feature flags plugin. The implementation is clean, maintains backward compatibility, and includes comprehensive tests and documentation. The architectural approach of separating mark and shake phases is sound.

Status: ✅ Recommended for approval with minor suggestions

---

Strengths

1. Excellent Documentation

   • Comprehensive README updates with clear examples
   • Well-documented TypeScript types with JSDoc
   • Thorough inline code comments explaining the two-phase workflow
   • Clear migration path from old to new API

2. Backward Compatibility

   • Graceful fallback to legacy BuildTimeConfig in packages/feature-flags/src/lib.rs:54-60
   • Existing users won't experience breaking changes
   • Sensible defaults (mark mode as default)

3. Test Coverage

   • 8 new WASM integration tests covering both modes
   • Tests for edge cases (exclude flags, nested scopes, multiple libraries)
   • Snapshot testing for regression detection
   • All tests passing ✅

4. Clean Architecture

   • Unified FeatureFlagsConfig API that dispatches to appropriate transforms
   • Good separation of concerns between mark and shake modes
   • DRY principle followed with reusable components

---

Issues & Suggestions

1. Potential Configuration Validation Issue ⚠️

Location: packages/feature-flags/src/lib.rs:30-53

The plugin doesn't validate mode-specific required fields:

• Mark mode requires libraries but config has #[serde(default)] making it optional
• Shake mode requires flagValues but it's also optional with default

Impact: Users could configure shake mode without flagValues, resulting in no transformations occurring silently.

Suggestion:

match config.mode {
    TransformMode::Mark => {
        if config.libraries.is_empty() {
            panic!("Mark mode requires at least one library in 'libraries' config");
        }
        // ... rest of code
    }
    TransformMode::Shake => {
        if config.flag_values.is_empty() {
            panic!("Shake mode requires at least one flag value in 'flagValues' config");
        }
        // ... rest of code
    }
}

2. Documentation Inconsistency 📝

Location: crates/swc_feature_flags/src/config.rs:40 and TypeScript types line 325

The comment says "Only used in mark mode" for marker_object, but this is incorrect:

• Mark mode uses it to generate markers
• Shake mode uses it to identify markers to replace

Suggestion: Update comments to:

/// Global object name for markers (default: "__SWC_FLAGS__")
/// Used in both modes: mark mode generates these markers, shake mode consumes them

3. Type Definitions Have Inconsistent Comments 📝

Location: packages/feature-flags/types.d.ts:633

The comment says "Only used in shake mode" for markerObject in the unified config, which conflicts with the Rust implementation where it's used in both modes.

Suggestion: Keep documentation consistent between Rust and TypeScript.

4. Potential Edge Case: Empty Config 🤔

Location: packages/feature-flags/src/lib.rs:29-30

If users provide an empty object {}, it will parse as valid FeatureFlagsConfig with mark mode and empty libraries, then silently do nothing.

Suggestion: Add validation or at minimum a warning comment in docs about this scenario.

5. Missing Test Case 🧪

The WASM tests don't verify the two-phase workflow end-to-end (running mark then shake in sequence). This is the primary use case described in the documentation.

Suggestion: Add a test like:

test("Should support two-phase workflow (mark then shake)", async () => {
  // Phase 1: Mark
  const { code: markedCode } = await transformCode(input, { mode: "mark", libraries: {...} });
  
  // Phase 2: Shake
  const { code: finalCode } = await transformCode(markedCode, { mode: "shake", flagValues: {...} });
  
  expect(finalCode).not.toContain("__SWC_FLAGS__");
  expect(finalCode).toMatchSnapshot();
});

6. Config Field Usage Clarity 📋

Location: Multiple locations in config structs

Some fields are marked as "not used" in certain modes, but they still accept values. This could confuse users who set flagValues in mark mode and wonder why nothing happens.

Consideration: Document this behavior more prominently in error messages or validation.

---

Performance Considerations ⚡

Positive:

• No performance regressions expected
• Mode-based dispatch is efficient (single match statement)
• Boxing the Pass trait in feature_flags_pass is necessary and acceptable
• Dead code elimination in shake mode will improve bundle sizes

Note: The Box<dyn Pass> return type in crates/swc_feature_flags/src/lib.rs:169 is required due to different concrete types returned from each branch. This is the idiomatic approach.

---

Security Considerations 🔒

Status: ✅ No security concerns identified

• No unsafe code introduced
• No user input directly executed
• Marker object name validation via serde ensures safe identifiers
• DCE transformation is deterministic and safe
• No risk of code injection

---

Best Practices ✨

Well Done:

• ✅ Follows Rust idioms (derive macros, trait implementations)
• ✅ Proper use of #[serde(default)] for optional fields
• ✅ Comprehensive error messages with context
• ✅ Consistent naming conventions (camelCase for JS/TS, snake_case for Rust)
• ✅ Good use of snapshot testing for transformation verification

Minor Suggestions:

• Consider adding #[non_exhaustive] to the TransformMode enum for future extensibility
• The default_true() and default_marker_object() functions are nice for consistency

---

Test Coverage Summary 📊

Current Coverage:

• ✅ Mark mode: marker generation, custom markers
• ✅ Shake mode: DCE, logical operations
• ✅ Edge cases: exclude flags, nested scopes, multiple libraries
• ✅ Configuration: default mode behavior

Missing Coverage:

• ⚠️ Two-phase end-to-end workflow
• ⚠️ Error cases (invalid configs, empty configs)
• ⚠️ Marker object mismatch between phases

Recommendation: Add tests for error cases to ensure graceful failures.

---

Recommendations for Merge 🎯

Required Changes:

1. Add validation for mode-specific required fields (Issue feat(plugins): Determine project structures #1)
2. Fix documentation inconsistency about marker_object usage (Issues Add transform for emotion #2, feat(styled-components): Implement transforms #3)

Recommended Changes:
3. Add two-phase end-to-end test (Issue #5)
4. Add validation/warning for empty config scenario (Issue #4)

Optional:
5. Add error handling tests
6. Consider more descriptive panic messages

---

Conclusion

This is a solid PR that adds valuable functionality with good backward compatibility. The two-phase approach is architecturally sound and well-documented. With the suggested validation improvements, this will be production-ready.

Great work on the comprehensive documentation and test coverage! 🎉

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

[claude]

PR Review: Two-Phase Feature Flag Transformation

Summary

This PR adds a well-designed two-phase approach (mark & shake modes) for feature flag transformation. The implementation is solid with good documentation and test coverage. Below are my detailed findings.

---

✅ Strengths

Architecture & Design

1. Clean separation of concerns: The two-phase approach (mark → shake) is well-architected and provides clear value for different build scenarios
2. Backward compatibility: Excellent handling of legacy BuildTimeConfig through fallback parsing in packages/feature-flags/src/lib.rs:54-61
3. Unified config structure: FeatureFlagsConfig elegantly combines both modes while maintaining clear field usage boundaries
4. Strong type safety: TypeScript definitions in types.d.ts are comprehensive with excellent JSDoc examples

Documentation

1. Exceptional README: The updated README is thorough with clear examples of both modes and complete workflow demonstrations
2. Inline documentation: Good Rust doc comments explaining the purpose and usage of each mode
3. Example-driven: Multiple real-world examples showing mark mode, shake mode, and the complete two-phase workflow

Testing

1. Comprehensive WASM tests: 8 tests covering both modes, edge cases (exclude flags, nested scopes), and multiple libraries
2. Good test organization: Tests are well-structured by feature area (mark mode, shake mode, edge cases, etc.)
3. Snapshot testing: Proper use of snapshots to catch unexpected output changes

---

🔍 Issues & Suggestions

1. Bug: Potential Config Parsing Issue ⚠️

Location: packages/feature-flags/src/lib.rs:30

The config parsing logic tries FeatureFlagsConfig first, then falls back to BuildTimeConfig. However, this could be problematic:

if let Ok(config) = serde_json::from_str::<FeatureFlagsConfig>(config_str) {

Problem: Since FeatureFlagsConfig has all fields marked with #[serde(default)], it will successfully parse even when given a BuildTimeConfig (just with empty/default values for mode-specific fields). This means the fallback to BuildTimeConfig at line 54 may never be reached.

Impact: Users with existing BuildTimeConfig configurations might get unexpected behavior (mark mode with their libraries config) instead of the intended build-time transformation.

Recommendation: Consider one of these approaches:

• Require a mode field to distinguish new vs. legacy config
• Use a tagged enum wrapper for config deserialization
• Document that legacy configs should include "mode": "mark" for clarity
• Add integration tests that verify legacy config parsing works correctly

2. Unclear Mode Requirements 📝

Location: crates/swc_feature_flags/src/config.rs:30-31, crates/swc_feature_flags/src/config.rs:45

The comments say:

• "Library configurations... Required in mark mode, not used in shake mode"
• "Flag values to apply... Required in shake mode"

Issues:

• In shake mode, libraries might still be needed if you want to both mark flags from source AND shake them in one pass (though unlikely)
• The word "Required" suggests validation, but there's no validation logic enforcing these requirements
• Users could create invalid configs (shake mode without flagValues) that would fail silently or produce unexpected results

Recommendation:

• Add config validation in both the Rust crate and TypeScript plugin
• Consider returning clear error messages for invalid mode + field combinations
• Example: "Shake mode requires flagValues to be specified"

3. Inconsistent Marker Removal 🔧

Location: packages/feature-flags/src/lib.rs:46

In shake mode, remove_markers is hardcoded to true:

remove_markers: true,

But this doesn't respect what users might want to configure. What if they want markers to remain for debugging or further processing?

Recommendation: Expose removeMarkers as a configuration option in FeatureFlagsConfig for shake mode, or document why it's intentionally fixed.

4. Missing Error Handling ⚠️

Location: packages/feature-flags/src/lib.rs:56-57

let config = serde_json::from_str::<BuildTimeConfig>(config_str)
    .expect("invalid config: must be either FeatureFlagsConfig or BuildTimeConfig");

Issues:

• expect() causes a panic with limited context for debugging
• The error message doesn't show what was invalid about the config
• Users won't know which fields are wrong or what format is expected

Recommendation:

let config = serde_json::from_str::<BuildTimeConfig>(config_str)
    .unwrap_or_else(|e| panic!(
        "Failed to parse plugin config. Expected FeatureFlagsConfig or BuildTimeConfig.\nError: {}\nConfig: {}", 
        e, 
        config_str
    ));

5. Unrelated Change 🤔

Location: contrib/mut-cjs-exports/src/local_export_strip.rs:214

-
 }

A blank line was removed in an unrelated file. This should ideally be in a separate cleanup commit or removed from this PR to keep changes focused.

6. Test Coverage Gaps 📊

While the tests are good, some scenarios are missing:

Missing test cases:

• Shake mode without specifying libraries (should this work?)
• Mark mode without specifying libraries (should show clear error?)
• Shake mode without flagValues (what happens?)
• Custom markerObject in shake mode (does it match correctly?)
• Mixing mark and shake in unexpected ways
• Config validation error messages
• Legacy BuildTimeConfig parsing (to ensure backward compatibility actually works)

Recommendation: Add tests for error cases and edge conditions, especially around config validation.

7. Type Safety: markerObject Mismatch Risk 🎯

Location: Config usage in both modes

If users run mark mode with markerObject: "__CUSTOM__" and then shake mode with the default "__SWC_FLAGS__", the shake phase won't find any markers.

Recommendation:

• Document prominently that markerObject must match between phases
• Consider a validation warning if shake mode is used with a custom marker object
• Add an integration test showing both phases with custom marker objects

8. TypeScript Type Deprecation Notice 📚

Location: packages/feature-flags/types.d.ts:1399

@deprecated Use FeatureFlagsConfig with mode: "shake" instead

This deprecation suggests BuildTimeConfig should be replaced, but the deprecation message says to use "shake" mode, which is incorrect—it should suggest "mark" mode since BuildTimeConfig is the mark phase.

Correction needed:

@deprecated Use FeatureFlagsConfig with mode: "mark" instead

---

🔒 Security

No security concerns identified. The changes don't introduce:

• Command injection vectors
• Unsafe deserialization
• External input without validation (plugin configs are build-time only)

---

⚡ Performance

No significant performance concerns. The implementation:

• Maintains the same visitor pattern as before
• Doesn't add unnecessary allocations
• Uses efficient HashMap lookups for flag values

The two-phase approach may actually improve performance in some build pipelines by separating concerns.

---

📝 Code Quality

Positive:

• Clean Rust code following SWC patterns
• Good use of serde attributes for config parsing
• Consistent naming conventions
• Well-structured module organization

Minor suggestions:

1. Consider adding #[non_exhaustive] to config structs to allow future field additions without breaking changes
2. The feature_flags_pass function in crates/swc_feature_flags/src/lib.rs:163 returns Box<dyn Pass> - could use impl Pass for potentially better performance
3. Consider adding logging/tracing for debugging mode selection and config parsing

---

🎯 Summary & Recommendation

This is a high-quality PR that adds significant value to the feature flags plugin. The two-phase architecture is well-thought-out, and the documentation is excellent.

Main action items:

1. Critical: Fix the config parsing fallback issue (Issue feat(plugins): Determine project structures #1)
2. Important: Add config validation for mode-specific required fields (Issue Add transform for emotion #2)
3. Important: Add tests for backward compatibility and error cases (Issue test: Add internal test plugin for integration testing #6)
4. Minor: Fix the TypeScript deprecation message (Issue chore(ci): Use prebuilt version of swc-dev #8)
5. Minor: Remove the unrelated whitespace change (Issue chore: Configure CI #5)

Once these issues are addressed, this PR will be ready to merge. Great work on the implementation and documentation! 🚀

---

Test Results: ✅ PR states all tests passing (5 Rust tests + 8 WASM tests)

Breaking Changes: ✅ None - maintains backward compatibility

[claude]

Pull Request Review: feat(feature-flags): Add two-phase mode support (mark & shake)

Summary

This PR adds a unified two-phase transformation system for the feature flags plugin, introducing "mark" and "shake" modes. The implementation is well-structured and maintains backward compatibility while adding significant new functionality.

✅ Strengths

Architecture & Design

• Clean separation of concerns: The two-phase approach (mark → shake) is intuitive and follows a clear workflow
• Backward compatibility: Fallback to BuildTimeConfig ensures existing users won't break
• Unified configuration: FeatureFlagsConfig consolidates both modes into a single, coherent interface
• Good defaults: Mark mode as default is sensible for the two-phase workflow

Code Quality

• Well-documented: Comprehensive JSDoc comments, README examples, and inline documentation
• Consistent patterns: Uses established SWC patterns (visit_mut_pass, VisitMutWith)
• Type safety: Strong typing in both Rust (enums, structs) and TypeScript (.d.ts)

Testing

• Comprehensive test coverage: 8 WASM integration tests covering both modes
• Edge cases handled: Tests for excluded flags, nested scopes, multiple libraries, and shadowed variables
• Snapshot testing: Proper use of snapshots for output validation

Documentation

• README is excellent: Clear examples of both phases, when to use each mode, complete workflows
• TypeScript types: Detailed JSDoc with examples in types.d.ts
• Migration path: Clear deprecation notice on BuildTimeConfig

🔍 Issues & Concerns

1. Potential Configuration Confusion (Minor)

In shake mode, the libraries field is unused but still accepted in the config. This might confuse users:

packages/feature-flags/src/lib.rs:42-51

TransformMode::Shake => {
    // Phase 2: Substitute markers and perform DCE
    let runtime_config = RuntimeConfig {
        flag_values: config.flag_values,
        remove_markers: true,
        collect_stats: config.collect_stats,
        marker_object: config.marker_object,
    };
    // Note: config.libraries is ignored here

Recommendation: Consider validating configuration and warning/erroring when mode-specific fields are provided incorrectly:

• libraries should be required in mark mode, warn if provided in shake mode
• flagValues should be required in shake mode, warn if provided in mark mode

2. Deprecation Notice Inconsistency (Minor)

packages/feature-flags/types.d.ts:146

/**
 * @deprecated Use FeatureFlagsConfig with mode: "shake" instead
 */
export interface BuildTimeConfig

The deprecation message says "mode: shake" but should say "mode: mark" since BuildTimeConfig is the mark/build-time configuration.

3. Unrelated Code Change (Trivial)

contrib/mut-cjs-exports/src/local_export_strip.rs:214-217

The removal of a blank line in an unrelated file should ideally be in a separate commit or explained in the commit message. While harmless, it adds noise to the PR.

4. Missing Validation (Minor)

The shake mode doesn't validate that libraries configuration was used in a prior mark phase. If a user runs shake mode on code that hasn't been marked, it will silently do nothing.

Suggestion: Consider logging a warning or adding documentation about this potential pitfall.

5. Error Messages (Minor)

packages/feature-flags/src/lib.rs:57

.expect("invalid config: must be either FeatureFlagsConfig or BuildTimeConfig");

This error message could be more helpful by showing what was received or providing a link to documentation.

🎯 Best Practices Assessment

✅ Good Practices Observed

• Comprehensive documentation with examples
• Maintains backward compatibility
• Uses snapshot testing for output validation
• Proper use of #[serde(default)] for optional fields
• Clear naming conventions (Mark/Shake is intuitive)
• Added test script to package.json to build before testing

⚠️ Areas for Improvement

• Configuration validation could be stricter
• Some edge cases in error handling
• Could benefit from runtime warnings for misconfiguration

🔒 Security Considerations

No security concerns identified:

• No unsafe code introduced
• No external data sources without validation
• No credential handling
• Transformation logic appears safe

⚡ Performance Considerations

No performance concerns:

• Single-pass transformations in both modes
• No recursive or exponential complexity
• Efficient use of SWC's visitor pattern
• Box allocation in feature_flags_pass is reasonable for dynamic dispatch

📊 Test Coverage

Excellent coverage:

• ✅ Mark mode: marker generation, custom marker objects
• ✅ Shake mode: DCE, logical operations
• ✅ Configuration defaults
• ✅ Multiple libraries support
• ✅ Edge cases: exclude flags, nested scopes
• ✅ Snapshot testing for output verification

Potential gaps:

• No test for invalid configurations (e.g., shake mode without flagValues)
• No test for running shake mode on unmarked code
• No test for mismatched marker objects between phases

🎨 Code Style

Follows SWC project conventions well:

• Consistent use of snake_case in Rust, camelCase in TypeScript
• Proper use of #[serde(rename_all)]
• Good use of doc comments (/// in Rust, JSDoc in TypeScript)

📝 Recommendations

High Priority

1. Fix the deprecation message in types.d.ts (mark, not shake)
2. Add validation for mode-specific required fields

Medium Priority

3. Add warning when mode-inappropriate fields are provided
4. Improve error messages with actionable guidance
5. Consider adding tests for invalid configurations

Low Priority

6. Move the whitespace change to a separate cleanup commit
7. Add integration test showing full mark→shake pipeline
8. Document what happens if shake mode is run on unmarked code

✅ Final Verdict

APPROVED with minor suggestions for improvement.

This is a well-designed, well-implemented feature that significantly improves the plugin's usability. The two-phase approach is clear and well-documented. The code quality is high, and the test coverage is comprehensive.

The issues identified are minor and don't block the PR. The suggestions above would enhance the user experience but aren't critical for merging.

Great work! 🎉

---

Reviewed by: Claude Code
Review Date: 2026-01-23