Skip to content

fix: properly handle function-valued options in store.set#120

Merged
zbeyens merged 1 commit intomainfrom
fix/function-options-handling
Oct 16, 2025
Merged

fix: properly handle function-valued options in store.set#120
zbeyens merged 1 commit intomainfrom
fix/function-options-handling

Conversation

@zbeyens
Copy link
Member

@zbeyens zbeyens commented Oct 16, 2025

Summary

This PR fixes the handling of function-valued options in store.set() to correctly distinguish between:

  • Updater functions: Functions passed to update primitive/object values (should be invoked with current value)
  • Function values: Functions stored as actual state values (should NOT be invoked)

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:

  1. If the passed value is a function
  2. If the previous value exists and is not null
  3. If the previous value is NOT already a function

This ensures:

  • Updater callbacks work correctly: store.set('count', (prev) => prev + 1)
  • Function values are stored directly: store.set('handler', () => 'result')

Changes

  • Modified createBaseApi.ts:44-52 to add conditional logic for updater invocation
  • Added test coverage for both callback updates and function value storage
  • Added changeset documenting the fix

Test Coverage

  • ✅ Callback updates for primitive values still work
  • ✅ Function values can be stored without invocation
  • ✅ Null/undefined function values handled correctly

🤖 Generated with Claude Code

@changeset-bot
Copy link

changeset-bot bot commented Oct 16, 2025

🦋 Changeset detected

Latest commit: 01e0f8c

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

This PR includes changesets to release 1 package
Name Type
zustand-x Patch

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

@zbeyens
Copy link
Member Author

zbeyens commented Oct 16, 2025

🔄 Incremental Review - Initial Commit

Commit: 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:

  1. Callback updates test - Validates updater functions still work for primitives
  2. 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 Documentation

Proper 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 the shouldInvokeUpdater 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)
const shouldInvokeUpdater = /* ... */

💡 Edge Case Question:
What happens when trying to use an updater function on an initial undefined value?
Example: store.set('newKey', (prev) => prev + 1) where newKey doesn't exist yet?

  • Current logic: Won't invoke (prevValue is undefined) ✅ Correct behavior

Final Assessment

Status: ✅ 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 with Claude Code

@zbeyens zbeyens merged commit 87b85c6 into main Oct 16, 2025
1 check passed
@zbeyens zbeyens deleted the fix/function-options-handling branch October 16, 2025 15:36
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.

1 participant