🔄 Incremental Review - Initial CommitCommit:01e0f8c - fix: properly handle function-valued options in store.set Scope: Initial PR creation - all changes reviewed
Code Quality Analysis✅ Core Logic Implementation (createBaseApi.ts:44-52)Strengths: - Clean, well-structured conditional logic
- Proper null/undefined safety checks
- Maintains backward compatibility with updater functions
- Clear variable naming (
shouldInvokeUpdater)
Minor Observations: - Import consolidation (
TCreatedStoreMutateType, TState) is a good cleanup - Logic correctly handles edge cases: undefined, null, and existing function values
✅ Test Coverage (createStore.spec.ts)Comprehensive test suite added: - Callback updates test - Validates updater functions still work for primitives
- Function value storage test - Validates functions can be stored without invocation
- Tests both required and optional function handlers
- Uses
vi.fn() spies to verify functions aren't called - Tests null → function transition case
Strengths: - Excellent edge case coverage (null optional handlers)
- Proper type safety with typed store state
- Verifies both positive and negative assertions
✅ Changeset DocumentationProper semantic versioning with patch-level change documented.
Review Checklist- ✅Code Correctness: Logic properly differentiates updater vs. value functions
- ✅Test Coverage: Comprehensive tests for both use cases
- ✅Backward Compatibility: Existing updater functionality preserved
- ✅Type Safety: Proper TypeScript types in tests
- ✅Documentation: Changeset properly describes the fix
- ✅Edge Cases: Handles undefined, null, and function-valued prevValue
- ✅Code Style: Follows project conventions
- ✅No Breaking Changes: Pure bug fix, maintains API contract
Potential Considerations💡 Suggestion - Documentation Enhancement (Optional):
Consider adding JSDoc comments to theshouldInvokeUpdater logic explaining the behavior: // Only invoke as updater when:// 1. Value is a function// 2. Previous value exists (not undefined/null)// 3. Previous value is NOT a function (to allow storing function values)constshouldInvokeUpdater=/* ... */ 💡 Edge Case Question:
What happens when trying to use an updater function on an initial undefined value?
Example:store.set('newKey', (prev) => prev + 1) wherenewKey doesn't exist yet? - Current logic: Won't invoke (prevValue is undefined) ✅ Correct behavior
Final AssessmentStatus: ✅Approved - Excellent Implementation This is a well-thought-out fix that: - Solves the core problem elegantly
- Maintains full backward compatibility
- Includes comprehensive test coverage
- Handles all edge cases correctly
- Follows clean code principles
No blocking issues identified. The implementation is production-ready.
Reviewed:01e0f8c 🤖 Generated withClaude Code |
Summary
This PR fixes the handling of function-valued options in
store.set()to correctly distinguish between:Problem
Previously,
store.set()would invoke ANY function passed as a value, making it impossible to store function references as state values. This caused issues when trying to store callbacks, handlers, or other function-valued options.Solution
Added logic to detect when a value should be treated as an updater function vs. a function value by checking:
This ensures:
store.set('count', (prev) => prev + 1)store.set('handler', () => 'result')Changes
createBaseApi.ts:44-52to add conditional logic for updater invocationTest Coverage
🤖 Generated withClaude Code