useFollowValue

[mattgperry]

Closes #3322

[claude]

Code Review: useAnimatedValue Hook

I have reviewed this PR which introduces useAnimatedValue, a generalized hook for creating animated MotionValues with support for all animation types (spring, tween, inertia, keyframes). This addresses issue 3322.

Strengths

1. Excellent refactoring approach with clean separation of concerns
2. Comprehensive test coverage (393 lines) covering all major scenarios
3. Proper TypeScript overloading and type safety
4. Good documentation with clear JSDoc comments
5. Follows existing patterns consistently

Critical Issue

Missing source in useInsertionEffect dependency array (use-animated-value.ts:77-79). If source changes from a MotionValue to a primitive or vice versa, the effect will not re-run, causing stale subscriptions.

Recommendation: Add source to dependencies array

Other Issues

1. Potential memory leak in promise chain for animation events (animated-value.ts:107-110)
2. Type safety issue with any cast in use-spring.ts:44
3. Accessing private API via string indexing for events notification

Security & Performance

Security: No concerns identified
Performance: Appropriate use of frame.postRender and proper cleanup

Summary

Well-implemented feature with excellent refactoring approach. The critical dependency array issue should be addressed before merging. Overall, this is a solid PR that adds valuable functionality.

[claude]

PR Review: useFollowValue Hook

Summary

This PR introduces a new useFollowValue hook that generalizes the functionality of useSpring to support all animation transition types (spring, tween, inertia, keyframes). The implementation refactors useSpring to use useFollowValue internally, eliminating code duplication.

✅ Strengths

1. Excellent Test Coverage

• Comprehensive test suite covering multiple scenarios (393 lines of tests)
• Tests for different animation types (spring, tween)
• Tests for both number and string values with units (%, etc.)
• Event lifecycle tests (animationStart, animationComplete)
• Memory leak prevention tests (unsubscribe behavior)
• Edge cases like jump() and different transition configurations

2. Clean Architecture

• Good separation of concerns: core logic in motion-dom, React integration in framer-motion
• Proper code reuse: useSpring now delegates to useFollowValue
• springValue and attachSpring correctly refactored to use followValue and attachFollow

3. Type Safety

• Proper TypeScript overloads for different input types
• Correct type exports and narrowing for string vs number MotionValues

4. API Design

• Intuitive naming that clearly conveys the "follow" behavior
• Consistent with existing Motion API patterns
• Backward compatible refactoring of useSpring

🐛 Potential Issues

1. Missing Event Cleanup (Memory Leak Risk)

Location: packages/motion-dom/src/value/follow-value.ts:106-112

The animationStart and animationComplete event notifications are triggered without cleanup:

frame.postRender(() => {
    startAnimation()
    value["events"].animationStart?.notify()
    activeAnimation?.then(() => {
        value["events"].animationComplete?.notify()
    })
})

Problem: If the component unmounts or the animation is stopped before completion, the promise chain could trigger after cleanup, potentially notifying on a destroyed value.

Recommendation:

frame.postRender(() => {
    startAnimation()
    value["events"].animationStart?.notify()
    activeAnimation?.then(() => {
        // Check if value is still active before notifying
        if (value["events"].animationComplete) {
            value["events"].animationComplete.notify()
        }
    }).catch(() => {
        // Handle cancellation silently
    })
})

2. Race Condition in Rapid Updates

Location: packages/motion-dom/src/value/follow-value.ts:76-90

When values change rapidly, latestValue and latestSetter might be updated while an animation is in flight:

const startAnimation = () => {
    stopAnimation()
    const currentValue = asNumber(value.get())
    const targetValue = asNumber(latestValue)  // latestValue could change before animation starts
    // ...
}

Since frame.postRender() defers execution, latestValue could be updated multiple times before startAnimation() executes.

Recommendation: Capture the target value at call time or use a queue to handle rapid updates properly.

3. Inconsistent Default Behavior

Location: packages/motion-dom/src/value/follow-value.ts:90-96

The comment states "Default to spring if no type specified" but this isn't explicitly enforced in the type system:

activeAnimation = new JSAnimation({
    keyframes: [currentValue, targetValue],
    velocity: value.getVelocity(),
    // Default to spring if no type specified (matches useSpring behavior)
    type: "spring",  // Only applies if options.type is undefined
    restDelta: 0.001,
    restSpeed: 0.01,
    ...options,  // Could override type to undefined
    onUpdate: latestSetter,
})

Recommendation: Ensure type defaults are properly enforced:

const transitionOptions = {
    type: "spring" as const,
    restDelta: 0.001,
    restSpeed: 0.01,
    ...options,
    velocity: value.getVelocity(),
}

4. Missing Unit Validation

Location: packages/motion-dom/src/value/follow-value.ts:67-69

Unit extraction is naive and doesn't validate that source and value units match:

const unit = typeof initialValue === "string"
    ? initialValue.replace(/[\d.-]/g, "")
    : undefined

Example Issue: If value starts as "10px" but source changes to "50%", the animation will incorrectly use px units throughout.

Recommendation: Either validate unit consistency or extract units from the current source value on each animation.

5. Type Narrowing Issue in useFollowValue

Location: packages/framer-motion/src/value/use-spring.ts:42

The cast to any bypasses type safety:

export function useSpring(
    source: MotionValue<string> | MotionValue<number> | string | number,
    options: SpringOptions = {}
) {
    return useFollowValue(source as any, { type: "spring", ...options })
}

Recommendation: Add proper overloads to useFollowValue or refactor to maintain type safety without any.

🎯 Performance Considerations

1. JSON.stringify in Dependency Array

Location: packages/framer-motion/src/value/use-follow-value.ts:78

useInsertionEffect(() => {
    return attachFollow(value, source, options)
}, [value, JSON.stringify(options)])

Concern: JSON.stringify(options) runs on every render. For complex options or frequent renders, this could be expensive.

Recommendation: Consider using a custom comparison function or useMemo to stabilize the options object.

2. Animation Check Optimization

Location: packages/motion-dom/src/value/follow-value.ts:85-88

if (currentValue === targetValue) {
    return
}

Good early exit, but this only checks numeric equality. For string values with units, this works due to asNumber(), but it would be more explicit to add a comment explaining this behavior.

📝 Documentation & Code Style

1. Excellent JSDoc Comments

The documentation is clear and includes practical examples. Well done!

2. Minor: Inconsistent Export Pattern

• useFollowValue is exported from the main index
• But followValue (the plain JS version) is also exported from the index

This is actually correct, but ensure both are documented in the public API docs.

🔒 Security Considerations

No security concerns identified. The code doesn't interact with user input directly or perform unsafe operations.

✨ Suggestions for Enhancement

1. Add Visual Regression Tests: While unit tests are comprehensive, consider adding visual tests for the different animation types to catch rendering issues.

2. Performance Benchmarks: Given the issue mentioned performance benefits of useSpring, it would be valuable to add benchmark tests comparing useFollowValue with different animation types.

3. Consider Animation Interruption Strategies: Allow users to configure how interruptions are handled (blend, replace, queue).

🎬 Conclusion

This is a well-implemented feature that addresses a genuine user need (issue #3322). The code quality is high, test coverage is excellent, and the architecture is sound. The concerns raised are mostly edge cases and potential improvements rather than blocking issues.

Recommendation: ✅ Approve with minor suggestions

The identified issues should be addressed, but they don't block merging:

• Priority 1: Fix potential memory leak in event cleanup
• Priority 2: Handle race conditions in rapid updates
• Priority 3: Improve type safety (remove as any)

Great work on this PR! 🎉

[claude]

PR Review: useFollowValue Hook

This PR introduces useFollowValue, a generalized animation hook that addresses issue #3322 by providing support for multiple animation types (spring, tween, inertia, keyframes) with a unified API.

Strengths

1. Well-structured refactoring: useSpring now delegates to useFollowValue
2. Comprehensive test coverage for multiple animation types
3. Good documentation with practical examples
4. Proper layer separation between motion-dom and framer-motion
5. Consistent with codebase conventions

Critical Issues

1. Memory Leak in Subscription Cleanup (follow-value.ts:102-128)

The attach() call on line 102 only registers stopAnimation as cleanup, but when source is a MotionValue, the subscriptions created on lines 116-120 are returned from attachFollow but not from the passive effect cleanup. This may cause memory leaks.

Fix: Consolidate cleanup into a single function passed to value.attach() that handles both animation stop and subscription removal.

2. JSON.stringify in Dependency Array (use-follow-value.ts:79)

Using JSON.stringify(options) in useInsertionEffect dependencies fails for non-serializable values (functions, driver) and causes unnecessary re-renders.

Suggestion: Remove from deps and document that options should be stable/memoized.

3. Race Condition in Animation Events (follow-value.ts:106-112)

Animation events are fired in frame.postRender without checking if the animation/value still exists. If cleanup happens before postRender executes, this accesses destroyed state.

Fix: Add an isActive flag that's set to false during cleanup.

Other Suggestions

• Test name on line 220 says 'immediate=true' but uses jump() method - name is misleading
• Consider handling edge cases for complex string values like calc()
• Add tests for unmounting during animation and rapid source changes

Overall Assessment

Solid PR that addresses the feature request well. Main concerns are memory leaks and race conditions that should be fixed before merging.

Recommendation: Request changes for issues #1 and #3.

[claude]

Code Review: useFollowValue Hook

I've reviewed PR #3485 which introduces the useFollowValue hook as requested in issue #3322. Overall, this is a well-executed implementation that successfully addresses the feature request. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Architecture & Code Reuse

   • The refactoring of useSpring and springValue to use the new generic implementation is clean and eliminates ~105 lines of duplicate code
   • The progression from attachFollow → followValue → useFollowValue follows the same pattern as existing spring implementations, maintaining consistency

2. Comprehensive Test Coverage

   • 339 lines of well-structured tests covering multiple scenarios (see use-follow-value.test.tsx:1)
   • Tests include: type validation, spring/tween animations, event handling, subscription management, and unit handling (numbers and percentages)
   • Good use of test helpers like syncDriver for deterministic animation testing
   • Tests for animation lifecycle events (animationStart, animationComplete)

3. Proper Type Safety

   • Function overloads correctly handle string vs number MotionValues (see use-follow-value.ts:47-64)
   • FollowValueOptions type properly omits lifecycle callbacks that don't make sense in the hook pattern (onUpdate, onComplete, etc.)

4. Good Documentation

   • JSDoc comments with clear examples showing spring, tween, and inertia usage (see use-follow-value.ts:17-44)
   • Naming is intuitive: "follow" conveys the behavior of tracking another value

5. Proper Resource Management

   • Subscription cleanup is handled correctly in attachFollow (see follow-value.ts:115-128)
   • The test at line 203 specifically validates that old subscriptions are cleaned up when switching sources

---

🔍 Areas for Improvement

1. Potential Animation Event Timing Issue (Medium Priority)

Location: follow-value.ts:106-112

frame.postRender(() => {
    startAnimation()
    value["events"].animationStart?.notify()
    activeAnimation?.then(() => {
        value["events"].animationComplete?.notify()
    })
})

Issue: The animationStart event is notified synchronously after startAnimation(), but activeAnimation might be null if the animation early-returned (lines 85-88 when currentValue === targetValue). This could cause animationComplete to never fire.

Recommendation: Move event notification inside startAnimation() or check if animation was created:

frame.postRender(() => {
    startAnimation()
    if (activeAnimation) {
        value["events"].animationStart?.notify()
        activeAnimation.then(() => {
            value["events"].animationComplete?.notify()
        })
    }
})

2. Type Safety: Accessing Private API (Low Priority)

Location: follow-value.ts:108, use-follow-value.test.tsx:220

Using value["events"] to access private members bypasses TypeScript's visibility modifiers. While this works, consider:

• Adding a protected/internal method for event notification
• Or documenting this as an intentional internal API pattern

3. Missing Edge Case: Unit Mismatch (Low Priority)

Location: follow-value.ts:67-70, 115-117

When tracking a source MotionValue, if the source changes from one unit to another (e.g., "10px" → "50%"), the current implementation could produce invalid values. The unit is determined once at initialization and never updated.

Recommendation: Either:

• Document this limitation
• Or handle unit changes by re-extracting the unit when source changes

4. Test Coverage Gap: Tween Animation (Low Priority)

The removed test "creates a tween animation when type is tween" (commit message mentions it was removed as flaky) leaves a gap. While you have configuration tests, there's no actual behavioral test for tween animations completing.

Recommendation: Consider adding a tween test with a more deterministic setup (similar to the spring test with syncDriver).

---

📊 Performance Considerations

The implementation looks performant:

• ✅ Animations are properly stopped before starting new ones (line 80)
• ✅ No-op optimization when target equals current value (lines 85-88)
• ✅ useInsertionEffect is used appropriately in React hook (see use-follow-value.ts:76)
• ✅ Dependency on JSON.stringify(options) is acceptable for this use case (line 78)

One minor note: The JSON.stringify(options) dependency could cause unnecessary re-subscriptions if options are created inline. Consider documenting that users should memoize options objects for optimal performance.

---

🔒 Security Considerations

No security concerns identified. The implementation:

• Properly handles user input (numeric/string values)
• Uses appropriate type guards (isMotionValue)
• No eval or dynamic code execution

---

🎯 Naming & API Design

The final name useFollowValue is good, but I'm curious about the evolution:

• Started as useAnimatedValue (more generic)
• Changed to useFollowValue (more descriptive)

Rationale for current name: "Follow" clearly conveys the tracking behavior, distinguishing it from the transform/derive pattern. Good choice!

Minor suggestion: Consider also exporting a convenience hook like useAnimatedValue(value, options) that defaults to common tween usage, since springs are already covered by useSpring. But this is optional - the current API is sufficient.

---

✅ Verification Against Requirements

Checking against issue #3322:

• ✅ Supports multiple animation types (spring, tween, inertia, keyframes)
• ✅ Returns a MotionValue with automatic subscription management
• ✅ Type-safe API
• ✅ Declarative usage pattern
• ✅ Performance considerations addressed
• ✅ Consistent with existing library patterns

---

📝 Summary

This is a solid implementation that successfully addresses the feature request. The code is well-tested, follows existing patterns, and maintains backward compatibility by refactoring useSpring to use the new implementation.

Recommended actions before merge:

1. ⚠️ Must fix: Animation event timing issue (see point Initial API (WIP) #1 above) to ensure events fire correctly
2. ✅ Nice to have: Add a deterministic tween animation test
3. ✅ Nice to have: Document unit mismatch limitation or handle it

The first point is the only critical issue - the rest are minor improvements that could be addressed in follow-up PRs.

Overall assessment: ✅ Approve with minor fixes recommended

Great work on this feature! It fills an important gap in the API and does so in a clean, maintainable way.

---

Reviewed with assistance from Claude Code following CLAUDE.md guidelines