Skip to content

Conversation

@som-sm
Copy link
Collaborator

@som-sm som-sm commented Oct 13, 2025

@som-sm som-sm requested a review from sindresorhus October 13, 2025 07:41
@claude
Copy link

claude bot commented Oct 13, 2025

Pull Request Review: If - Add note regarding tail recursion

Thank you for this documentation enhancement! This is a valuable addition that will help users avoid performance pitfalls.

Strengths

  1. Addresses a real problem: As demonstrated in PR 1266, this is a practical issue that developers can encounter when using If in recursive type implementations.

  2. Clear, practical example: The Includes type example effectively demonstrates both the problem and the solution with realistic code.

  3. Concrete demonstration: Using StringRepeat with 100 repetitions provides a concrete way to reproduce the issue.

  4. Actionable guidance: The note clearly explains when to avoid If and what to use instead.

Code Quality and Best Practices

Documentation Structure: Good - follows type-fest patterns with proper examples and imports.

Minor Issue - Placement: The note appears after all the primary examples. Consider moving it closer to the top, after the existing Note section at lines 9-11, with a brief bullet point referencing the detailed example below. This would improve discoverability.

Potential Issues

  1. Example complexity: The example introduces IsEqual, which might be unfamiliar to users. Consider adding a brief comment explaining what IsEqual does.

  2. Comment formatting: Line 76 has a visual error indicator with tildes that might not render well in all contexts. Consider simplifying to: // @ts-expect-error -- Type instantiation is excessively deep and possibly infinite

Test Coverage

Status: Not applicable - this is a documentation-only change.

Optional suggestion: Consider adding test cases to test-d/if.ts that demonstrate the examples work as expected. This would verify the example code actually compiles correctly.

Performance Considerations

This change has no runtime impact and actually helps users write more performant types. The guidance is sound from a TypeScript compiler perspective.

Security Concerns

None - documentation-only change.

Suggestions for Improvement

  1. Add a brief note in the main description mentioning performance considerations for recursive types.

  2. Consider simplifying the example or adding more context about why tail recursion matters for readers unfamiliar with the concept.

  3. Add cross-reference to ArrayReverse as a real-world example of tail-recursive implementation.

Verdict

This is a valuable documentation enhancement that addresses a real issue discovered during development. The example is practical and the guidance is correct.

Recommendation: Approve with minor suggestions

The minor suggestions above would improve clarity and discoverability, but the PR is fundamentally sound and ready to merge as-is if the maintainer prefers.

Checklist:

  • Code quality and best practices: Documentation follows type-fest conventions
  • Potential bugs or issues: No bugs; minor formatting suggestions
  • Performance considerations: Actually improves performance awareness
  • Security concerns: None
  • Test coverage: Could add example validation tests (optional)

Great contribution! This will help developers avoid the pitfall you discovered.

@sindresorhus sindresorhus merged commit 18c67e1 into main Oct 13, 2025
7 checks passed
@sindresorhus sindresorhus deleted the doc/add-note-to-if-type branch October 13, 2025 11:46
@claude
Copy link

claude bot commented Oct 13, 2025

PR Review Summary

I've reviewed this pull request that adds documentation about tail recursion limitations with the If type. Here's my analysis:

✅ Strengths

  1. Valuable Educational Content: The added documentation addresses an important performance consideration when using the If type in recursive contexts. This is exactly the kind of practical guidance that helps users avoid common pitfalls.

  2. Clear Before/After Example: The side-by-side comparison of the non-tail-recursive Includes type vs. the tail-recursive IncludesWithoutIf effectively demonstrates the problem and solution.

  3. Concrete Demonstration: Using StringRepeat<'0', 100> provides a tangible test case that users can verify, and the @ts-expect-error annotation correctly documents the expected failure.

  4. Proper Imports: The example correctly imports required types (If, IsEqual, StringRepeat) and follows the repository's convention of using type imports.

  5. Formatting Consistency: The documentation follows the existing style with proper indentation, inline comments with //=>, and clear explanations.

📝 Minor Observations

  1. Documentation Placement: The new note appears after the existing examples but before the @category tags. This placement is appropriate, though it makes the documentation quite lengthy. The content is valuable enough to justify the length.

  2. Type Parameter Names: The example uses S and Char as type parameters. While the CLAUDE.md guidelines suggest descriptive names over single letters, Char is descriptive and S is contextually clear as a string parameter. This is acceptable.

  3. No Test File Changes: The PR only modifies documentation without adding corresponding test cases. However, since this is documenting a limitation rather than new functionality, and the example itself serves as inline documentation of the behavior, this is reasonable.

🎯 Alignment with Repository Guidelines

  • ✅ Follows the documentation style (no * prefixes)
  • ✅ Includes realistic examples with //=> comments
  • ✅ Uses descriptive type parameter names (mostly)
  • ✅ Proper .d.ts file structure maintained
  • ✅ References related discussion (PR Add ArrayReverse type #1266)

🔍 Technical Accuracy

The explanation of tail recursion is technically sound:

  • The If type creates an intermediate conditional that prevents TypeScript's tail call optimization
  • Using extends true directly in the conditional enables tail recursion
  • The depth limit issue ("Type instantiation is excessively deep") is correctly identified

📊 Overall Assessment

This is a high-quality documentation improvement that adds significant value for users working with recursive types. The PR successfully addresses feedback from PR #1266 and provides actionable guidance on when to avoid using If in favor of direct conditionals.

Rating: ⭐⭐⭐⭐⭐ Excellent documentation enhancement

The merge was appropriate, and this addition will help prevent users from encountering mysterious depth instantiation errors when building recursive types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants