fix: gesture onStart, onUpdate, and onEnd (et al) should be worklets

[nmassey]

What: the bug

The onStart, onUpdate, and onEnd gesture handlers are not being automatically workletized. This causes these handlers to run on the JS thread instead of the UI thread, and this is causing a number of issues, including animations running more slowly and some data (via useSharedData) being set asynchronously, leading to some race conditions.

(See more discussion below in "Discussion of the bug".)

What: fix the code

Add "worklet" directives for each gesture handler function.

What: fix the tests

The tests (in src/hooks/usePanGestureProxy.test.tsx) also include onBegin and onFinalize handlers. Since the tests include these, I decided to add some functions to support them in src/hooks/usePanGestureProxy.ts using the same method as for onStart, onUpdate, and onEnd.

If some handlers for a given gesture are workletized and some are not, RNGH will show a runtime error like the following:

[react-native-gesture-handler] Some of the callbacks in the gesture are worklets and some are not. Either make sure that all calbacks are marked as 'worklet' if you wish to run them on the UI thread or use '.runOnJS(true)' modifier on the gesture explicitly to run all callbacks on the JS thread.

Let's make sure that that error doesn't appear in our tests.

Ideally, we could test to ensure that the handler functions actually end up workletized. This would be a good follow-on TODO.

Discussion of the bug

This [lack of workletization] seems to have been accidentally introduced in:

• Develop #507

In particular, the following change
https://github.com/dohooo/react-native-reanimated-carousel/pull/507/files#diff-519aac449991f2daef8d2cf263d4dcc9cb7c067210ac7e1d218cf9f31881f890L365-L368

old code (worked to correctly workletify) - 4.0.0-alpha.5 and earlier

the handlers were set up via Gesture.Pan().onBegin(onGestureBegin)...

https://github.com/dohooo/react-native-reanimated-carousel/blame/5e3c3015346994eafb8c898a4ba1d345568f5ee2/src/components/ScrollViewGesture.tsx#L365-L368

Each handler function (e.g. onGestureBegin) was set up as a "worklet", e.g. https://github.com/dohooo/react-native-reanimated-carousel/blame/5e3c3015346994eafb8c898a4ba1d345568f5ee2/src/components/ScrollViewGesture.tsx#L264-L266

new code (borked, not automatically workletified) - 4.0.0-alpha.6 and later

the handlers are set up via const gesture = Gesture.Pan(); then later gesture.onStart(... here
https://github.com/dohooo/react-native-reanimated-carousel/blob/main/src/hooks/usePanGestureProxy.ts#L79-L97

The onStart / onUpdate / onGestureEnd do not include the "worklet" directive at the start of their functions.

But why doesn't the react-native-reanimated/plugin babel plugin automatically workletify these handlers?

Unfortunately, that plugin can only automatically workletify functions when they are defined via Gesture.Pan().onStart (or similar). The fact that we instead define const gesture = Gesture.Pan(); in one place and then later call gesture.onStart() / gesture.onUpdate() / gesture.onEnd() means that the babel plugin doesn't find these and automatically workletify them.

Source: https://github.com/software-mansion/react-native-reanimated/blob/main/plugin/src/gestureHandlerAutoworkletization.ts

Screengrab video recordings

These recordings were taken on a simulator and include console.log output indicating whether each call to a gesture handler is running in a worklet or not. The code to output these console log messages is shown here:

• nmassey@21fc44a

non-worklet behavior on 4.0.0-alpha.10 (unpatched except for console.log messages)

See that all debug messages indicate that the code is not running inside of worklet.

Untitled.mov

improved behavior with patch

See that all debug messages indicate the the code is running inside of worklet.

Untitled3.mov

possible TODOs

1. DRY up onStart / onUpdate / onEnd etc. code in usePanGestureProxy.ts
2. possibly add in other handlers? e.g. onChange
3. add a test to make sure the gesture handlers are workletified (I'm not sure of a good way to do this except possibly via some gray box testing, e.g. adding some code within the component to validate)

[changeset-bot]

🦋 Changeset detected

Latest commit: 17621ca

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

This PR includes changesets to release 1 package

Name	Type
react-native-reanimated-carousel	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

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-native-reanimated-carousel	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 6, 2024 4:28pm

[nmassey]

Note: this PR may also resolve the underlying issues for:

1. fix: onGestureEnd -> endWithSpring uses outdated data via useSharedValue #574
2. fix: flicker when starting pan: panOffset value race condition #576

That being said, I still recommend those PRs in addition to this PR for code clarity's sake.

[sieeniu]

when it would be merged?

[dohooo]

Incredible work, thank you! But before merging, could you gen a changeset for this PR?

npx changeset

[Bekaxp]

Hi, is there a reason why this PR fix was not yet merged in? It's been open and stale for quite some time... Thank you for your amazing work!

[qwertychouskie]

@dohooo Would deeply appreciate if this and #648 are merged. Would be great to get these fixes in before we push the new version of our app to prod. If there's anything I can do to help, please let me know. (FWIW I already became a GitHub Sponsor out of my own pocket)

[nmassey]

hi @qwertychouskie - I've been using yarn patch to unblock me while waiting for these PRs to be merged 😕
Are you on the Discord?