part I. Strip complicated but low priority features ahead of reanimated conversion#149
part I. Strip complicated but low priority features ahead of reanimated conversion#149thomasttvo wants to merge 11 commits intomasterfrom
reanimated conversion#149Conversation
| minZoom?: number; | ||
| doubleTapDelay?: number; | ||
| doubleTapZoomToCenter?: boolean; | ||
| bindToBorders?: boolean; |
There was a problem hiding this comment.
TypeScript types include removed prop causing silent failure
The panBoundaryPadding prop remains in the TypeScript interface definition, but the implementation has been completely removed (the applyPanBoundariesToOffset helper file was deleted and all related logic stripped out). TypeScript users can pass this prop without compilation errors, but it will be silently ignored at runtime. This causes unexpected behavior for users who set panBoundaryPadding expecting it to control pan boundaries.
Additional Locations (1)
elliottkember
left a comment
There was a problem hiding this comment.
I think this should be fine, but it's a breaking change so we'll need a minor version, and a CHANGELOG.md or some kind of breaking change notice in the release. I'll be interested to see whether anybody is using these features and has trouble upgrading
|
@elliottkember that's a good point. I've created this PR to create a better setup for changelog generation. We can edit the changelog (once it's generated) to explain further which features have been dropped once we publish. Additionally, we can also publish this as part of an RC version together with the reanimated stuff and ask the community for feedback before turning it into a official version |
reanimated conversion
|
@thomasttvo Sounds like a plan. It may actually be worth making a new major version RC. Given the scope of the reanimated change I think it's justified, and will avoid anybody accidentally updating their if they use |
reanimated conversionreanimated conversion
|
@claude review |
1 similar comment
|
@claude review |
|
@claude please review this PR |
|
@claude review |
| @@ -500,8 +403,6 @@ class ReactNativeZoomableView extends Component< | |||
| this.panAnim.stopAnimation(); | |||
There was a problem hiding this comment.
🔴 In _handlePanResponderGrant, this.panAnim.stopAnimation() is called without a callback, so when a native-driver animation is interrupted mid-flight the JS-side offsetX/offsetY fields are never updated to reflect where the animation actually stopped. This PR converted offsetX/offsetY from getter/setter pairs (which tracked state via __offsets) to plain class fields, removing the last mechanism that could compensate for this gap; now every subsequent pan gesture or tap calculation starts from a stale offset, causing a visible content jump. Fix: replace the bare call with this.panAnim.x.stopAnimation(x => { this.offsetX = x; }); this.panAnim.y.stopAnimation(y => { this.offsetY = y; });.
Extended reasoning...
Background: what the PR changed
PR #149 simplified offsetX/offsetY from getter/setter pairs backed by __offsets state:
private set offsetX(x: number) { this.__setOffset('x', x); }
private get offsetX() { return this.__getOffset('x'); }into plain class fields initialised to 0. Previously, the setter wrote through to component state, giving the fields a chance to be refreshed on re-render. Now they are raw JS values that must be kept in sync manually.
Why stopAnimation() without a callback is a problem
Animated.ValueXY.stopAnimation() (and the underlying Animated.Value.stopAnimation()) accepts an optional callback that receives the animation's current value at the moment it is stopped. When useNativeDriver: true is used the animation runs entirely on the native thread; the JS thread only receives values via the addListener mechanism, which fires asynchronously and can lag behind by several frames. Calling stopAnimation() without a callback therefore leaves the JS-side panAnim value wherever it happened to be at the last listener tick, which may be well short of the visual position.
The listener does not help
componentDidUpdate adds a listener:
this.panAnim.addListener(() => this._invokeOnTransform());This listener only calls _invokeOnTransform() (an external callback for consumers). It does not update this.offsetX or this.offsetY, so the plain fields remain at whatever value they had before the animation started.
Two distinct stale-offset paths
-
_resolveAndHandleTapstarts a 200 ms timing animation withuseNativeDriver: trueand, in its completion callback, calls_updateStaticPin(). It never writes back tooffsetX/offsetY. If the user's finger lands during this window,_handlePanResponderGrantfires, calls the barestopAnimation(), and the fields stay at the pre-animation values. -
moveStaticPinTowrites the target values intooffsetX/offsetYbefore starting the animation:
this.offsetX = contentWidth / 2 - position.x + pinX / this.zoomLevel;
this.offsetY = ...;
Animated.timing(this.panAnim, { toValue: { x: this.offsetX, y: this.offsetY }, useNativeDriver: true, duration }).start();If interrupted, stopAnimation() leaves the visual position somewhere in the middle while offsetX/offsetY already hold the destination — a different kind of stale value, equally wrong.
Step-by-step proof (tap-animation case)
- View is at rest;
offsetX = 0,panAnim.x._value = 0. - User double-taps;
_resolveAndHandleTapfires. It computestoValue = { x: 100, y: 0 }and starts a 200 ms native animation.offsetXis not changed. - At t = 80 ms the native thread has visually advanced the view to x ≈ 40. The JS listener has received a value of x ≈ 35.
- User's finger touches the screen.
_handlePanResponderGrantfires, callsthis.panAnim.stopAnimation()(no callback). The animation stops at x ≈ 40 visually. offsetXis still0.panAnim.x._valueis ≈ 35 (last listener tick).- On the next move event
_handleShiftingcomputesoffsetX + shift.x = 0 + shift.x, anchoring the pan fromx = 0, notx = 40. The content jumps ~40 logical pixels back to the start offset.
Fix
Replace:
this.panAnim.stopAnimation();With:
this.panAnim.x.stopAnimation(x => { this.offsetX = x; });
this.panAnim.y.stopAnimation(y => { this.offsetY = y; });The callback is invoked synchronously with the current JS-side value, ensuring offsetX/offsetY match the stopped position before any subsequent pan math runs.
| private offsetX = 0; | ||
| private offsetY = 0; |
There was a problem hiding this comment.
🔴 The panAnim.addListener and zoomAnim.addListener calls added in the constructor (to sync the new offsetX/offsetY/zoomLevel plain fields introduced by this PR) and in componentDidUpdate (to invoke _invokeOnTransform) are never removed in componentWillUnmount, creating a memory leak and potential post-unmount setState calls. When external animated values are passed via zoomAnimatedValue/panAnimatedValueXY props, those values outlive the component, so stale listeners on the dead instance continue firing. Fix by storing the listener IDs returned by addListener and calling removeListener in componentWillUnmount.
Extended reasoning...
This PR introduces two new addListener calls in the constructor to keep the new plain offsetX/offsetY/zoomLevel fields in sync with animation values. However, componentWillUnmount only clears the measureZoomSubjectInterval and never removes any animation listeners, creating a memory leak and a source of post-unmount side effects.
Affected listeners (four total):
- in the constructor — NEW in this PR, syncs and
- in the constructor — NEW in this PR, syncs
- in — pre-existing, calls
- in — pre-existing, calls
Why existing code does not prevent it: only contains . There is no call to , , or any cleanup of listener IDs. The ids returned by are discarded.
The critical scenario — external animated values: When a consumer passes or props, the component wires its internal / to those external values. Those external objects belong to the parent and outlive the child component. After the child unmounts, the external animated value still holds references to the stale listeners, which continue to fire on every animation tick.
Step-by-step proof:
- Parent creates and passes to .
- In the constructor, runs, then adds two listeners (constructor listener + later componentDidUpdate listener) to the parent-owned value. The returned IDs are not stored.
- The component unmounts (e.g. user navigates away). fires but only clears the interval — the listeners on remain registered.
- The parent continues animating (e.g. driving some other UI). Every update triggers the stale listeners on the dead component instance.
- The listener calls , which eventually calls , which calls on an unmounted component — a React warning and potential crash in strict mode.
Fix: Store the IDs returned by in instance variables and call / (or ) inside .
| | zoomStep | number | How much zoom should be applied on double tap | 0.5 | | ||
| | pinchToZoomInSensitivity | number | the level of resistance (sensitivity) to zoom in (0 - 10) - higher is less sensitive | 3 | | ||
| | pinchToZoomOutSensitivity | number | the level of resistance (sensitivity) to zoom out (0 - 10) - higher is less sensitive | 1 | | ||
| | movementSensibility | number | how resistant should shifting the view around be? (0.5 - 5) - higher is less sensitive | 1.9 | |
There was a problem hiding this comment.
🟡 The README props table (line 162) documents movementSensibility with a default of 1.9, but the actual defaultProps in src/ReactNativeZoomableView.tsx sets it to 1. This PR explicitly rewrote the props table but left the stale 1.9 value uncorrected -- the fix is a one-line README change: 1.9 to 1.
Extended reasoning...
The README.md props table at line 162 documents the movementSensibility prop with a default value of 1.9, while the actual defaultProps in src/ReactNativeZoomableView.tsx sets it to 1.
This discrepancy is confirmed in the compiled output files (lib/commonjs/ReactNativeZoomableView.js and lib/module/ReactNativeZoomableView.js), both of which show movementSensibility: 1.
This PR explicitly rewrote the props table in README.md -- removing rows for bindToBorders, panBoundaryPadding, animatePin, and disableMomentum -- yet preserved the stale 1.9 default value for movementSensibility. This was a missed opportunity to correct a pre-existing documentation error.
The impact is primarily on developer experience: a user reading the docs expects panning to be approximately twice as dampened as it actually is out of the box. Additionally, if a developer sets movementSensibility: 1.9 explicitly to match the documented default, they silently alter the behavior from the actual default of 1.
Step-by-step proof:
- A developer reads the README and sees movementSensibility defaults to 1.9.
- They call ReactNativeZoomableView without specifying movementSensibility.
- The component uses defaultProps.movementSensibility = 1, not 1.9 as documented.
- The developer notices panning feels different than expected based on the docs.
The fix is a one-line change to README.md line 162: replace 1.9 with 1.
| const transform = [ | ||
| { translateY: -pinSize.height }, | ||
| { translateX: -pinSize.width / 2 }, | ||
| ...pinAnim.getTranslateTransform(), | ||
| ]; | ||
|
|
||
| const opacity = pinSize.width && pinSize.height ? 1 : 0; |
There was a problem hiding this comment.
🟣 In StaticPin.tsx, onPanResponderMove uses && to check both dx and dy thresholds, but onPanResponderRelease correctly uses ||. This asymmetry means a purely horizontal or vertical pan starting on the pin is silently swallowed — not forwarded to the parent for panning, and not recognized as a tap. Since this PR already modifies StaticPin.tsx, it would be a good opportunity to fix this pre-existing one-character bug by changing && to || on line 56.
Extended reasoning...
What the bug is: onPanResponderMove (line 56) guards the parent-forwarding call with Math.abs(gestureState.dx) > 5 && Math.abs(gestureState.dy) > 5 — both axes must exceed 5px. But onPanResponderRelease (line 60) uses Math.abs(gestureState.dx) > 5 || Math.abs(gestureState.dy) > 5 — either axis exceeding 5px is enough to conclude the gesture was a drag, not a tap. These two conditions are logically inconsistent.
The specific code path that triggers it: A user places their finger on the StaticPin and drags horizontally (e.g., scrolling the map left/right). During the drag, gestureState.dx grows large but gestureState.dy stays near zero. The && condition in onPanResponderMove evaluates to false, so onParentMove is never called — the map does not pan. When the user lifts their finger, onPanResponderRelease sees dx > 5, the || condition is true, and it returns early without firing onPress or onLongPress either. The gesture silently disappears.
Why existing code does not prevent it: The PanResponder is set up in a useRef so it captures onParentMove at mount time. The asymmetric operators likely passed code review because both lines look superficially similar and the bug only manifests for axis-aligned gestures, which may be less common in diagonal-dominant test scenarios.
Impact: Any user who starts a pan gesture on the static pin — rather than on an empty area of the map — and moves predominantly in one axis will find the map unresponsive to that gesture. On typical map UIs where horizontal panning is extremely common, this is a noticeable UX defect. The pin effectively acts as a dead zone for single-axis panning.
Step-by-step proof:
- User touches the
StaticPin—onStartShouldSetPanResponderreturnstrue, recordstapTime. - User drags finger 15px right, 2px down —
gestureState.dx = 15,gestureState.dy = 2. onPanResponderMovefires:Math.abs(15) > 5 && Math.abs(2) > 5evaluates totrue && false=false.onParentMoveis NOT called. Map stays frozen.- User releases finger.
onPanResponderReleasefires:Math.abs(15) > 5 || Math.abs(2) > 5evaluates totrue || false=true. Early return. NeitheronPressnoronLongPressfires. - Result: the gesture is completely ignored — no pan, no tap, no long press.
How to fix it: Change line 56 from && to || so both handlers share the same definition of 'this gesture has moved enough to be a drag'.
Since we'll convert to
reanimatedin a later PR, it's important to strip some features to avoid a complicated migration. We can always reintroduce them in thereanimatedworld if we need to.Note
Remove pan-boundary/momentum and pin animation features, simplify offsets/gestures, and update API, docs, and builds accordingly.
applyPanBoundariesToOffsetand related animation helpers.bindToBorders,panBoundaryPadding,disableMomentum, andanimatePin; removepinAnimfromStaticPin; methods no longer acceptbindToBordersparams.useLatestCallbackhook.getZoomToAnimationin animations; add package type fields in build outputs.Written by Cursor Bugbot for commit c674720. Configure here.