Skip to content

part I. Strip complicated but low priority features ahead of reanimated conversion#149

Open
thomasttvo wants to merge 11 commits intomasterfrom
thomas/strip-features
Open

part I. Strip complicated but low priority features ahead of reanimated conversion#149
thomasttvo wants to merge 11 commits intomasterfrom
thomas/strip-features

Conversation

@thomasttvo
Copy link
Copy Markdown
Collaborator

@thomasttvo thomasttvo commented Dec 20, 2025

Since we'll convert to reanimated in a later PR, it's important to strip some features to avoid a complicated migration. We can always reintroduce them in the reanimated world if we need to.

Note

Remove pan-boundary/momentum and pin animation features, simplify offsets/gestures, and update API, docs, and builds accordingly.

  • Core changes:
    • Remove pan-boundary logic, momentum decay, and pin animations; simplify offset handling and gesture flow; delete applyPanBoundariesToOffset and related animation helpers.
    • Modernize code (optional chaining, JSX runtime) and streamline internal state.
  • API/Props:
    • Drop bindToBorders, panBoundaryPadding, disableMomentum, and animatePin; remove pinAnim from StaticPin; methods no longer accept bindToBorders params.
    • Update typings and d.ts accordingly; add useLatestCallback hook.
  • Docs/Examples/Build:
    • Revise README tables/usages and example app to reflect removed props.
    • Keep only getZoomToAnimation in animations; add package type fields in build outputs.

Written by Cursor Bugbot for commit c674720. Configure here.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment @cursor review or bugbot run to trigger another review on this PR

minZoom?: number;
doubleTapDelay?: number;
doubleTapZoomToCenter?: boolean;
bindToBorders?: boolean;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Collaborator

@elliottkember elliottkember left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Base automatically changed from thomas/RN79 to master December 24, 2025 20:01
@thomasttvo thomasttvo added the breaking Breaking changes label Jan 2, 2026
@thomasttvo
Copy link
Copy Markdown
Collaborator Author

thomasttvo commented Jan 2, 2026

@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

@thomasttvo thomasttvo changed the title Strip complicated but low priority features Strip complicated but low priority features ahead of reanimated conversion Jan 2, 2026
@elliottkember
Copy link
Copy Markdown
Collaborator

@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 ^2 in package.json.

@thomasttvo thomasttvo changed the title Strip complicated but low priority features ahead of reanimated conversion part I. Strip complicated but low priority features ahead of reanimated conversion Mar 24, 2026
@thomasttvo
Copy link
Copy Markdown
Collaborator Author

@claude review

1 similar comment
@thomasttvo
Copy link
Copy Markdown
Collaborator Author

@claude review

@thomasttvo
Copy link
Copy Markdown
Collaborator Author

@claude please review this PR

@thomasttvo
Copy link
Copy Markdown
Collaborator Author

@claude review

@@ -500,8 +403,6 @@ class ReactNativeZoomableView extends Component<
this.panAnim.stopAnimation();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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

  1. _resolveAndHandleTap starts a 200 ms timing animation with useNativeDriver: true and, in its completion callback, calls _updateStaticPin(). It never writes back to offsetX/offsetY. If the user's finger lands during this window, _handlePanResponderGrant fires, calls the bare stopAnimation(), and the fields stay at the pre-animation values.

  2. moveStaticPinTo writes the target values into offsetX/offsetY before 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)

  1. View is at rest; offsetX = 0, panAnim.x._value = 0.
  2. User double-taps; _resolveAndHandleTap fires. It computes toValue = { x: 100, y: 0 } and starts a 200 ms native animation. offsetX is not changed.
  3. 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.
  4. User's finger touches the screen. _handlePanResponderGrant fires, calls this.panAnim.stopAnimation() (no callback). The animation stops at x ≈ 40 visually.
  5. offsetX is still 0. panAnim.x._value is ≈ 35 (last listener tick).
  6. On the next move event _handleShifting computes offsetX + shift.x = 0 + shift.x, anchoring the pan from x = 0, not x = 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.

Comment on lines +79 to +80
private offsetX = 0;
private offsetY = 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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):

  1. in the constructor — NEW in this PR, syncs and
  2. in the constructor — NEW in this PR, syncs
  3. in — pre-existing, calls
  4. 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:

  1. Parent creates and passes to .
  2. In the constructor, runs, then adds two listeners (constructor listener + later componentDidUpdate listener) to the parent-owned value. The returned IDs are not stored.
  3. The component unmounts (e.g. user navigates away). fires but only clears the interval — the listeners on remain registered.
  4. The parent continues animating (e.g. driving some other UI). Every update triggers the stale listeners on the dead component instance.
  5. 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 |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. A developer reads the README and sees movementSensibility defaults to 1.9.
  2. They call ReactNativeZoomableView without specifying movementSensibility.
  3. The component uses defaultProps.movementSensibility = 1, not 1.9 as documented.
  4. 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.

Comment on lines 38 to 43
const transform = [
{ translateY: -pinSize.height },
{ translateX: -pinSize.width / 2 },
...pinAnim.getTranslateTransform(),
];

const opacity = pinSize.width && pinSize.height ? 1 : 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. User touches the StaticPinonStartShouldSetPanResponder returns true, records tapTime.
  2. User drags finger 15px right, 2px down — gestureState.dx = 15, gestureState.dy = 2.
  3. onPanResponderMove fires: Math.abs(15) > 5 && Math.abs(2) > 5 evaluates to true && false = false. onParentMove is NOT called. Map stays frozen.
  4. User releases finger. onPanResponderRelease fires: Math.abs(15) > 5 || Math.abs(2) > 5 evaluates to true || false = true. Early return. Neither onPress nor onLongPress fires.
  5. 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'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants