Add useTransform signature with motion values#3466
Conversation
Adds a new signature to useTransform that accepts an object map of output
ranges and returns an object of motion values with the same keys:
const { opacity, x } = useTransform(inputValue, inputRange, {
opacity: [0, 1],
x: [100, 200]
})
The keys are captured once on first render, ensuring hooks are called in a
consistent order across renders. This allows the hook to be safely used
within loops.
Code Review for PR #3466SummaryThis PR adds a new signature to ✅ Strengths
🔍 Potential IssuesCritical: Motion Value Creation PatternLocation: The implementation directly instantiates // Current PR approach:
const motionValues = useConstant(() =>
keys.map((_, i) => new MotionValue(initialValues[i]))
)Issue: Looking at Recommendation: import { motionValue } from "motion-dom"
const motionValues = useConstant(() =>
keys.map((_, i) => motionValue(initialValues[i]))
)This ensures consistency and may be important for proper initialization (the factory might do additional setup). Moderate: Dependency Management for Transform FunctionsLocation: const transformers = keys.map((key) =>
transform(inputRange, outputMap[key], options)
)Issue: The Scenario: If Example: const [max, setMax] = useState(200)
const { opacity } = useTransform(x, [0, max], { opacity: [0, 1] })
// When max changes, updateValues uses new transformer
// But the subscription still references old transformersRecommendation: Either:
Minor: Synchronous Update TimingLocation: updateValues()Observation: This synchronous call during render ensures DOM styles are current, matching the pattern in Question: Could there be a scenario where Recommendation: Consider computing initial values lazily or ensuring they're always current. Minor: Memory EfficiencyLocation: const result: { [key: string]: MotionValue<O> } = {}
for (let i = 0; i < keys.length; i++) {
result[keys[i]] = motionValues[i]
}
return resultObservation: A new result object is created on every render. While JavaScript engines optimize this well, and the motion values themselves are stable, this could be cached. Recommendation: Consider using return useConstant(() => {
const result: { [key: string]: MotionValue<O> } = {}
for (let i = 0; i < keys.length; i++) {
result[keys[i]] = motionValues[i]
}
return result
})🧪 Testing SuggestionsThe test suite is excellent, but consider adding:
📝 DocumentationThe JSDoc is clear and well-written. One minor suggestion: emphasize in the * @remarks
*
* This is useful when you want to derive multiple values from a single input value.
* **IMPORTANT**: The keys of the output map must remain constant across renders to comply
* with React's Rules of Hooks.🎯 Overall AssessmentThis is a solid implementation that adds valuable functionality. The critical issue with motion value instantiation should be addressed, and the dependency management concern should be investigated. Once these are resolved, this will be a great addition to the API. Recommendation: Request changes for the motion value instantiation pattern, and consider addressing the transformer dependency issue before merging. |
Reuse the existing useTransform in a loop instead of duplicating logic. The keys are captured once with useConstant, ensuring hooks are called in consistent order across renders.
PR Review: Add useTransform signature with motion valuesOverviewThis PR adds a new signature to useTransform that accepts an object map of output ranges and returns an object of motion values with the same keys. This is a well-designed feature that improves DX when deriving multiple values from a single input. Strengths1. Well-Structured Implementation
2. Comprehensive Test Coverage
3. Excellent Documentation
Observations & Suggestions1. Performance Consideration 2. Rules of Hooks Compliance 3. Edge Case: Empty Output Map Potential IssuesNone identified. The implementation is solid and follows React best practices. Security ConcernsNo security issues identified. The implementation does not introduce any XSS, injection, or other vulnerabilities. Test Coverage AssessmentExcellent. The test suite covers all critical scenarios including initial value computation, value updates, motion value stability, different value types, transform options, key stability, input range reactivity, and TypeScript type inference. Recommendations
Verdict: ApprovedThis is a well-crafted feature with excellent test coverage and documentation. The implementation correctly handles React's rules of hooks and provides a valuable DX improvement for common use cases. The suggestions above are minor enhancements, not blockers. Great work! |
Pull Request Review: Add useTransform signature with motion valuesI've reviewed this PR that adds a new signature to useTransform allowing it to accept an object map of output ranges. Code Quality and Best Practices - STRONG
Minor Suggestions:
Potential Bugs - CRITICAL ISSUE FOUNDRules of Hooks Violation at lines 250-252: The implementation contains a conditional hook call pattern that violates React's Rules of Hooks. The number of hook calls depends on runtime data (number of keys in outputMap): for (const key of keys) {
output[key] = useTransform(inputValue, inputRange, outputMap[key], options)
}While keys is captured with useConstant, this is fragile because:
Recommended Fix: Add DEV mode assertion that validates Object.keys(outputMap) matches the captured keys on subsequent renders to catch violations early. Performance Considerations
Security ConcernsNone identified - pure computational hook with no external I/O. Test Coverage - EXCELLENT184 added lines testing all key scenarios. Suggested additions:
Additional Observations
RecommendationsBefore merging:
Nice to have:
VerdictWell-implemented feature with excellent tests and documentation. Main concern is the potential Rules of Hooks violation which should be addressed with runtime validation in DEV mode. Reviewed by Claude Code |
Code ReviewThis is a well-crafted PR that adds a useful new signature to useTransform for creating multiple transformed motion values from a single input. The implementation is clean and the test coverage is comprehensive. Strengths
ObservationsImplementation Efficiency: The useMapTransform function calls useTransform for each key in the output map (line 251). This creates multiple subscriptions to the same input value. While this works correctly, it could be slightly less efficient than a single subscription. However, this approach maintains code simplicity, reuses existing well-tested logic, and is unlikely to cause performance issues in practice. Type Detection: The isOutputMap detection at line 191-194 is clever but does not verify that object values are arrays at runtime. TypeScript types provide compile-time safety, so this is acceptable. Test Improvement: The test about maintaining keys across renders could be stronger by explicitly verifying that new keys are ignored after initial render. Edge Case: Consider what happens if outputMap is an empty object. SummaryNo bugs identified! Performance characteristics are good. No security concerns. Code follows repository conventions from CLAUDE.md. LGTM with minor suggestions! This is a solid PR that adds valuable functionality with excellent test coverage. The implementation is clean, type-safe, and follows project conventions. The suggestions above are optional improvements, not blockers. |
- Use scale [0.5, 1.5] so scale(1.5) is visible in transform - Use opacity [0, 0.5] with clamp:false to test extrapolation - Simplify stable values test to check output object reference
Code Review: Add useTransform signature with motion valuesThank you for this PR! This is a well-implemented feature that adds a convenient API for deriving multiple motion values from a single input. Here's my detailed review: ✅ Strengths
🔍 Observations & Suggestions1. Type Safety ConsiderationThe implementation at const isOutputMap =
outputRangeOrMap \!== undefined &&
\!Array.isArray(outputRangeOrMap) &&
typeof inputRangeOrTransformer \!== "function"Potential Issue: This could theoretically match unexpected inputs. For example, if someone passes a non-array object as Suggestion: Consider adding a more defensive check: const isOutputMap =
outputRangeOrMap \!== undefined &&
\!Array.isArray(outputRangeOrMap) &&
typeof outputRangeOrMap === 'object' &&
outputRangeOrMap \!== null &&
typeof inputRangeOrTransformer \!== "function"2. Empty Object Edge CaseAt for (const key of keys) { // Empty loop if keys.length === 0
output[key] = useTransform(inputValue, inputRange, outputMap[key], options)
}Suggestion: Consider documenting that 3. Key Consistency WarningThe test at line 418-442 correctly verifies that keys are captured on first render. However, users might not realize this constraint. Suggestion: Consider adding a development-only warning if if (process.env.NODE_ENV \!== 'production') {
const currentKeys = Object.keys(outputMap)
if (currentKeys.length \!== keys.length ||
\!currentKeys.every((k, i) => k === keys[i])) {
console.warn(
'useTransform: outputMap keys changed between renders. ' +
'Keys are captured on first render and must remain consistent.'
)
}
}4. Test File - Async ConsistencyThe tests declare functions as
Suggestion: Remove 5. Performance ConsiderationThe implementation calls Note: This is not a concern for typical usage (2-5 keys), just worth documenting if this becomes a performance-critical path. 🛡️ Security & Stability
📋 Test Coverage AssessmentThe test suite covers:
Suggestion: Add tests for:
🎯 Final Recommendation✅ Approve with minor suggestions This is a solid implementation that adds real value to the API. The core logic is sound, properly tested, and follows best practices. The suggestions above are minor improvements for robustness and developer experience, but they're not blockers. The key strengths:
Great work! 🚀 |
Code Review SummaryThis PR adds a valuable new signature to ✅ Strengths
🔍 Minor Observations
🎯 Suggestions (Optional)
🔒 Security & Performance
📋 Test Coverage AssessmentThe test coverage is excellent, covering:
Minor test gap: Could add a test for an empty output map ✨ ConclusionThis is a high-quality PR that adds a useful feature with proper implementation, excellent tests, and good documentation. The code follows the repository's conventions (named exports, interfaces over types, arrow callbacks) as specified in CLAUDE.md. Recommendation: ✅ Approve - Ready to merge with optional minor enhancements above. Nice work! This will be a helpful addition to the API. 🎉 |
Adds a new signature to useTransform that accepts an object map of output ranges and returns an object of motion values with the same keys:
const { opacity, x } = useTransform(inputValue, inputRange, {
opacity: [0, 1],
x: [100, 200]
})
The keys are captured once on first render, ensuring hooks are called in a consistent order across renders. This allows the hook to be safely used within loops.