software-mansion / react-native-reanimated

React Native's Animated library reimplemented
https://docs.swmansion.com/react-native-reanimated/
MIT License
8.87k stars 1.3k forks source link

An ability to have JS handler along with animated handler simultaneously #6204

Open kirillzyusko opened 3 months ago

kirillzyusko commented 3 months ago

Description

I would like to have an ability to have 2 handlers (JS + reanimated) at the same time. Before I could achieve that using next code:

const CustomScrollView = ({onScroll: onScrollFromProps}) => {
  const onScroll = useAnimatedScrollHandler({
      onScroll: (event) => {
  }});

  return <Reanimated.ScrollView onScrollReanimated={onScroll} onScroll={onScrollFromProps} />
};

But at some point of time this construction stopped to work. I think it happens because of this: https://github.com/software-mansion/react-native-reanimated/blob/4115e83875bcd70becd5e02f770e4bd21c24294a/packages/react-native-reanimated/src/createAnimatedComponent/PropsFilter.tsx#L79-L86

Another option was to add such code:

const CustomScrollView = ({onScroll: onScrollFromProps}) => {
  const onScroll = useAnimatedScrollHandler({
      onScroll: (event) => {

      runOnJS(onScrollFromProps)({nativeEvent: event});
  }});

But in this case some of properties (currentTarget, cancellable, etc.) will be ignored.

Is there a recommended way to achieve this? 👀

Steps to reproduce

Add the code from description.

Snack or a link to a repository

N/A

Reanimated version

3.12.1

React Native version

0.74.2

Platforms

Android, iOS

JavaScript runtime

Hermes

Workflow

React Native

Architecture

Paper (Old Architecture)

Build type

Debug app & dev bundle

Device

iOS simulator

Device model

iPhone 15

Acknowledgements

Yes

github-actions[bot] commented 3 months ago

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

kirillzyusko commented 3 months ago

cc @tomekzaw as we discussed it with you in direct messages 🙃

tomekzaw commented 3 months ago

Thanks for reporting this issue, I've forwarded it internally. cc @tjzel @szydlovsky

kirillzyusko commented 2 months ago

I think the ideal solution would be to re-use useComposedEventHandlers hook and allow to include plain JS-callbacks 🤔

I haven't thought about the exact API yet, but I think a nice solution would be to have something like that:

const onScrollHandler1 = useAnimatedScrollHandler({
    onScroll(e) {
      console.log('Scroll handler 1 onScroll event');
    },
  });
const onScrollHandler2 = (e) => console.log(e);
const composedHandler = useComposedEventHandler([
    onScrollHandler1,
    { onScroll: onScrollHandler2, js: true },
  ]);
// ...
<Animated.ScrollView onScroll={composedHandler}>

And my idea how to achieve that was a modification of useComposedEventHandler:

const handler = useEvent<Event, Context>(...

handler.hasJSHandler = hasJSHandler(handlers)

return handler;

// ... and then in AnimatedComponent we need to check - if `hasJSHandler=true` then we don't need to substitute handler with `dummyListener` and we should use composed js-handler (i. e. propagate one event through all JS handers provided in `useComposedEventHandler`

Of course this is a very rough idea... If you have a better idea on how to achieve his - let me know, I'll be glad to hear that 👀 But my idea was re-using current API without bringing additional complexity to developers who is going to use that 🙈

But if you see a drawbacks in the proposed solution - let me know 👍

szydlovsky commented 2 months ago

@kirillzyusko I'll try building a PoC based on your idea, I will get back to you once I have some more intel on it 👍

kirillzyusko commented 2 months ago

By the way - I temporarily solved the problem by using useScrollViewOffset hook instead of useAnimatedScrollHandler. I was needed to know only the scrolled distance and do not perform any custom logic. And in this case useScrollViewOffset doesn't override onScroll property and I can use onScroll + useScrollViewOffset simulateneously.

But I think that the ability to have a composed mixture of worklet/js handlers still would be amazing feature 😎

szydlovsky commented 2 months ago

@kirillzyusko Hi again! Coming back to you after some digging. I have a very very early stage PoC and it seems like it may fly! I've found some limitations though:

  1. I had to specifically choose which events can be passed as JS handlers. For now, I've used the 5 scroll events (scroll, begin drag, end drag, begin momentum, end momentum). Do you think you would find a use-case for any other events? If so - which ones? Before I am able to tackle this problem, I can just directly try adding needed events (for now).
  2. It will be possible to use only one JS handler per JS event (which is quite understandable because of messing with props underneath). So far, I've come up with API like this:
    const composedHandler = useComposedEventHandler(
    [onDragHandler, onMomentumHandler],
    {
      onBeginDrag: (_e) => {
        console.log('[JS] begin drag');
      },
      onMomentumBegin: (_e) => {
        console.log('[JS] begin momentum');
      },
    }
    );

    So, the JS handlers are being passed in a similar (if not the same) object as in useAnimatedScrollHandler, as a separate second argument of useComposedEventHandler

Tell me what you think, I am open for suggestions 😄

szydlovsky commented 2 months ago

And a sneak peak of the PoC:

https://github.com/software-mansion/react-native-reanimated/assets/77503811/fb60a439-9f14-46f0-ac96-97dfd8acf6bb

kirillzyusko commented 2 months ago

I had to specifically choose which events can be passed as JS handlers. For now, I've used the 5 scroll events (scroll, begin drag, end drag, begin momentum, end momentum). Do you think you would find a use-case for any other events? If so - which ones? Before I am able to tackle this problem, I can just directly try adding needed events (for now).

I think all events that intercepted by useAnimatedScrollHandler (as you said - these 5 events) are enough 👍

It will be possible to use only one JS handler per JS event (which is quite understandable because of messing with props underneath). So far, I've come up with API like this:

I think it's not a limitation, but maybe it is possible to be consistent and have the API like:

const composedHandler = useComposedEventHandler(
    [onDragHandler, onMomentumHandler],
    [{ // <- an array of JS handlers
      onBeginDrag: (_e) => {
        console.log('[JS] begin drag');
      },
      onMomentumBegin: (_e) => {
        console.log('[JS] begin momentum');
      },
    },
    {onBeginDrag: props.onBeginDrag}]
  );

Could be probably convenient to have an ability to compose multiple JS handlers (for example if you want to combine a handler from props and your own). But this is just idea - will be glad to hear a feedback as well 🙂

szydlovsky commented 2 months ago

@kirillzyusko Alright, thanks for the response! I'll stick to the scrolling events for now. If it comes to the API, yeah, I think I might have an idea on how to merge multiple JS handlers as well - once again I'll let you know as soon as I figure something out 😄

szydlovsky commented 2 months ago

Hey @kirillzyusko I have the earliest PoC patch for you to try: reanimated_js_handlers.patch The api is currently what you asked for, namely, a second argument being an array of JS handlers. You can compose multiple ones. I am yet to correctly detect changes in JS handlers, so that might not work perfectly. Also, I am planning to use just one argument with a union type. Anyway, let me know what you think and I'd love some critical feedback 😄

kirillzyusko commented 2 months ago

@szydlovsky thank you for your hard work 💪

I tested changes from your PR and for me it looks like they work well 👀 (though I tested only basics scenarios)

Great job 🎉

szydlovsky commented 2 months ago

Ahh, just as I pushed some cool changes - now the hook should react to any changes in its argument, including JS handlers + just a single, union-typed argument. Nonetheless, here's the newest patch: rea_js_handlers_second.patch and I'm opening the feature to reviews 😄

kirillzyusko commented 2 months ago

@szydlovsky cool, I will try to test it tomorrow or over the weekend!

kirillzyusko commented 2 months ago

@szydlovsky I tested and everything works well! Thank you for your hard work!

szydlovsky commented 2 months ago

Super good news! I'll come back to you the last time when the PR get approved and ask if it still does well 😄