Skip to content
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

Make Fabric event receivers init only once #6907

Merged
merged 2 commits into from
Feb 4, 2025

Conversation

szydlovsky
Copy link
Contributor

Summary

Fixes #6896

When adding support for fabric, we have inadvertently kept previous listeners still active for fabric. This way, we registered twice with the same NodesManager for React Native's FabricEventDispatcher, resulting in pretty much all the events doubling in number on android. I added a simple check that takes care of it.

Test plan

Paste the following into EmptyExample and run FabricExample. Check logs making sure that both start and end are logged only once per gesture.

import React, { useState } from 'react';
import { View, Text } from 'react-native';
import Animated, { useAnimatedScrollHandler } from 'react-native-reanimated';

function EmptyExample() {
  const [w, sW] = useState(0);

  const scrollHandler = useAnimatedScrollHandler({
    onBeginDrag() {
      console.log('start');
    },
    onMomentumEnd() {
      console.log('end');
    },
  });

  return (
    <View
      style={{ flex: 1 }}
      onLayout={(evt) => sW(evt.nativeEvent.layout.width)}>
      <Animated.ScrollView
        onScroll={scrollHandler}
        horizontal
        snapToInterval={w}
        decelerationRate="fast">
        {Array.from({ length: 20 })
          .fill(0)
          .map((_, i) => {
            return (
              <View key={i} style={{ width: w }}>
                <Text>{i}</Text>
              </View>
            );
          })}
      </Animated.ScrollView>
    </View>
  );
}

export default EmptyExample;

Copy link
Member

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

Code looks good to me, haven't tested it though, let's wait for approvals from other reviewers

@Titozzz
Copy link
Contributor

Titozzz commented Feb 3, 2025

Just had that issue pop up on android, so I'm happy for that PR to exist, keeping an eye on it

Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

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

I've tested it and it seems to work well 👍

@piaskowyk piaskowyk enabled auto-merge February 4, 2025 10:41
@piaskowyk piaskowyk added this pull request to the merge queue Feb 4, 2025
Merged via the queue into main with commit d3e6ff2 Feb 4, 2025
11 checks passed
@piaskowyk piaskowyk deleted the @szydlovsky/fix-android-events-emitting branch February 4, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: onMomentumEnd called many times
5 participants