-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Events Handling] Make JS handlers mergeable along Worklet handlers #6268
base: main
Are you sure you want to change the base?
Conversation
const JSHandlers: JSHandlersObject<Event>[] = handlers | ||
.filter((h) => h !== null) | ||
.filter((h) => isJSHandler(h)) as JSHandlersObject<Event>[]; | ||
|
||
// Update/initialize the ref and determine whether JS handlers need rebuild or not | ||
if (JSHandlersRef.current === null) { | ||
JSHandlersRef.current = JSHandlers; | ||
} else { | ||
JSHandlersNeedRebuild = !areJSHandlersEqual( | ||
JSHandlersRef.current, | ||
JSHandlers | ||
); | ||
JSHandlersRef.current = JSHandlers; | ||
} | ||
|
||
// Setup the JSHandlersRecord object | ||
if (JSHandlers.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move that whole logic related to JS handler to another function?
packages/react-native-reanimated/src/hook/useComposedEventHandler.ts
Outdated
Show resolved
Hide resolved
type JSScrollEventsInput = | ||
| 'onScroll' | ||
| 'onBeginDrag' | ||
| 'onEndDrag' | ||
| 'onMomentumBegin' | ||
| 'onMomentumEnd'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these all the possible types of JS events? 🤔 Why are we only focusing on those events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently the only use-case needed for the upgrade (as requested). Thanks to this we can anyhow type the argument. Problem also lies in the fact that some events need different names to pass down to proper event handlers (for example, onBeginDrag needs to be translated to onScrollBeginDrag). I am yet to find a way to include all sensible events
JSHandlersRef.current, | ||
JSHandlers | ||
); | ||
JSHandlersRef.current = JSHandlers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do it here? Is it allowed by the rules of hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this hook doesn't comply to rules of hooks in the first place, but I believe it is a colossal task to make it compliant. It needs to calculate some thinks during render, because WorkletEventHandler
class applies stuff before it. Making it rules-of-hooks-friendly seems like a good idea as a refactor at some point..
if (JSHandlersRef.current === null) { | ||
JSHandlersRef.current = JSHandlers; | ||
} else { | ||
JSHandlersNeedRebuild = !areJSHandlersEqual( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to rely on useEffect
and the dependency array instead of implementing it on your own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to do that and failed with at least 3-4 different approaches. As mentioned earlier, we need its results before the render because of the way our event handling works in general (the logic of PropsFilter
and WorkletEventHandler
+ createAnimatedComponent
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And since it is an advanced hook and not many people are gonna use it, it doesn't need some super-tuned performance upgrades. For now it calculates everything it needs from scratch on every render and I didn't notice any slowdown caused by it
packages/react-native-reanimated/src/createAnimatedComponent/PropsFilter.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/src/createAnimatedComponent/PropsFilter.tsx
Outdated
Show resolved
Hide resolved
} else { | ||
props[key] = dummyListener; | ||
if (handler.eventNames.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this part of the code. It seems like if there is any JSHandler, we ignore all worklet event handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't! They don't need to be attached anywhere. The only logic concerning worklet event handlers was putting dummyListeners
in places for their respected props. Worklet event handlers are registered in native modules and don't need props to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we have JS handlers, we can't always just put dummyListeners
in the props because some JS handlers may need to use that. This is why there are 2 different approaches depending on JS handlers presence
const isWebHandler = has('listeners', handler); | ||
const hasJSHandlers = Object.keys(handler.JSHandlers).length > 0; | ||
|
||
if (hasJSHandlers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that a worklet event handler can also be a JS event handler? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, they are separate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to rework that API, instead of adding arguments let's allow composed structures as in my suggestion. Let's also stick to the names:
workletHandler
reactHandler
In all contexts just handler
should imply a workletHandler
rebuild = false, | ||
JSHandlers: Record<string, JSHandler<Event>> = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit ill structured to have JSHandlers
as the last argument. I'd suggest to change the API and relevant names to:
const handlers = {
onA: () => {}, // worklet handler
onB: {
workletHandler: () => {}, // worklet handler
reactHandler: () => {}, // react handler AKA JS handler
}
}
This way we'll have necessary backwards compatibility with the old API. This would require a slight change in the Reanimated Babel Plugin, but that's not a problem.
Summary
As per a request by @kirillzyusko #6204, I decided to take up the feature of using both Worklet and JS handlers at once.
API
It can be done through
useComposedEventHandler
hook with a following API:How it works?
The JS handlers passed to
useComposedEventHandler
are getting packed into a Record object and passed down the stream, throughuseEvent
hook all the way toWorkletEventHandler
object. There, they work in a different way depending on the platfrom. On mobile platforms, they are just kept there as a public field and then, thePropFilter
sets them on the corrseponding props, voila. On web,WorkletEventHandler
merges JS and Worklet handlers into its listeners if they respond to the same event. Then, thePropFilter
sets these listeners to the correct props. If theuseComposedEventHandler
gets no JS handlers as an argument, the old logic is kept.Limitations
The feature currently supports only scroll-based events for JS handlers (
onScroll
,onBeginDrag
,onEndDrag
,onMomentumBegin
,onMomentumEnd
).TODOS:
Test plan
Currently, I use a bit altered
useComposedEventHandler
examples to test it:Conditional merge code:
Different events merge:
Component's internal merge: