feat(flags): validate config and align docs

[kdy1]

Summary

• remove excludeFlags docs/types and clarify shake mode usage
• add config validation for required mark/shake options and non-empty functions
• add validation coverage in feature-flags tests

Testing

• pnpm --filter @swc/plugin-experimental-feature-flags test

[changeset-bot]

⚠️ No Changeset found

Latest commit: b68c34e

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]

Review of PR #584: feat(flags): validate config and align docs

Summary

This PR improves the feature-flags plugin by:

• Removing the unused excludeFlags option from docs and types
• Clarifying that shake mode operates on __SWC_FLAGS__ markers and doesn't use libraries
• Adding config validation to catch misconfigurations early
• Adding test coverage for the validation logic

Overall, this is a solid improvement that makes the API clearer and prevents runtime errors. However, there are a few concerns about the implementation approach.

---

Code Quality & Best Practices

Good:

• Clean documentation updates that accurately reflect the two-mode design
• Test coverage for the new validation logic
• Backward compatibility maintained for legacy BuildTimeConfig

Concerns:

1. Error handling approach (packages/feature-flags/src/lib.rs:16, 21, 28, 38, 42)

   Using panic!() for configuration validation is problematic:

   • Panics bypass Rust's error handling conventions
   • Makes error recovery impossible for consumers
   • Harder to test specific error messages
   • Poor user experience in production

   Recommendation: Consider returning Result<Program, String> or using a proper error type. If the plugin interface requires panics, at least document this behavior clearly.

2. Validation timing (packages/feature-flags/src/lib.rs:70, 97)

   Validation happens after deserialization succeeds, but there's a gap:

   • Invalid JSON structure is caught by serde
   • Valid JSON with invalid values (empty arrays) is caught by validation
   • But the error messages are different (serde vs panic)

   Recommendation: Consider using serde's validation features (e.g., custom deserializers) for more consistent error messages.

---

Potential Bugs

1. Validation inconsistency for shake mode (packages/feature-flags/src/lib.rs:26-33)

   The validation checks if libraries has empty functions in both mark and shake modes, but shake mode doesn't use libraries at all. This could be confusing if someone provides an empty libraries config in shake mode.

   Recommendation: Only validate libraries in mark mode:

   if config.mode == TransformMode::Mark {
       for (library, library_config) in &config.libraries {
           if library_config.functions.is_empty() {
               panic!(...);
           }
       }
   }

2. Test coverage gap (packages/feature-flags/tests/wasm.test.ts:193-205)

   The test validates that empty functions array is rejected, but doesn't test:

   • Empty libraries object in mark mode (vs missing libraries)
   • Empty flagValues object in shake mode (vs missing flagValues)
   • Having libraries in shake mode (should be allowed but ignored)

   Recommendation: Add tests for these edge cases to ensure validation is complete.

---

Performance Considerations

No performance concerns. The validation runs once during config parsing, which is negligible overhead.

---

Security Concerns

No security issues identified. The validation actually improves security by preventing misconfiguration.

---

Documentation Quality

Excellent improvements:

• The shake mode clarification is much clearer now
• Removing the excludeFlags section eliminates confusion about an unused feature
• The "Required in mark mode, not used in shake mode" comment on libraries is helpful

Minor suggestion:
Consider adding a note in the TypeScript types that libraries should be omitted (or can be empty) in shake mode for clarity:

/**
 * Library configurations: library name -> config
 *
 * Required in mark mode, should be omitted in shake mode.
 * ...
 */
libraries: Record<string, LibraryConfig>;

---

Test Coverage

Good coverage:

• Tests validate that mark mode requires libraries ✓
• Tests validate that shake mode requires flagValues ✓
• Tests validate that functions array must be non-empty ✓

Missing tests:

• Empty objects vs missing fields ({} vs undefined)
• Valid shake mode config without libraries field
• Mix of valid and invalid libraries

---

Recommendations Priority

High Priority:

1. Fix validation logic to only check libraries.functions in mark mode
2. Improve error handling (consider Result types or better error messages)

Medium Priority:
3. Add test cases for edge cases mentioned above
4. Update TypeScript types to clarify libraries should be omitted in shake mode

Low Priority:
5. Consider using serde validation for more consistent errors

---

Great work on improving the API surface and documentation! The changes make the plugin much easier to use correctly.

[Copilot]

Pull request overview

This PR improves the feature flags plugin by removing unused configuration options, clarifying mode-specific behavior, and adding configuration validation. The changes align documentation with actual implementation behavior, specifically clarifying that shake mode operates on __SWC_FLAGS__ markers and does not use the libraries configuration, while mark mode requires it.

Changes:

• Removed excludeFlags field from type definitions and documentation (unused feature)
• Added runtime configuration validation to ensure required fields are present for each mode
• Added test coverage for configuration validation scenarios

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/feature-flags/types.d.ts	Removed excludeFlags field from both FeatureFlagsConfig and BuildTimeConfig interfaces, added clarification that libraries are required in mark mode but not used in shake mode
packages/feature-flags/src/lib.rs	Added validate_feature_flags_config and validate_build_time_config functions to validate required configuration fields based on mode, integrated validation into plugin entry points
packages/feature-flags/tests/wasm.test.ts	Added comprehensive validation tests covering missing libraries in mark mode, missing flagValues in shake mode, and empty functions array
packages/feature-flags/README.tmpl.md	Removed excludeFlags documentation section, clarified shake mode behavior to emphasize it operates on markers and doesn't use libraries, updated usage examples
packages/feature-flags/README.md	Same documentation updates as README.tmpl.md

---

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

[Copilot]

The validation for non-empty functions in libraries should only apply in mark mode, not shake mode. According to the updated documentation, shake mode does not use libraries. This validation block should be moved inside the mark mode branch (after line 17) to prevent incorrect validation errors when a user provides a library config with empty functions in shake mode.