Giphy / giphy-react-native-sdk

GIPHY React Native SDK
https://developers.giphy.com
Apache License 2.0
67 stars 25 forks source link

Multiple GiphyDialog listeners conflict each other #164

Closed pierroo closed 5 months ago

pierroo commented 7 months ago

🐛 Bug Report

I have multiple screens in my app that use GiphyDialog.addListener which seems to be the only way to take action on the picked Giphy. Some of those screens are in the same navigation stack, meaning I can be in a screen that has such listener, and navigate to another screen that also uses its own giphy listener => such navigation won't unmount the previous one obviously (and I don't want to; navigation.navigate vs navigation.replace)

PROBLEM is: if the new screen I am into triggers the listener, it will also trigger the underlying listener of the previous screen in the stack that is still listening!

Obviously not what I want, since they both are supposed to target a very different purpose and task.

To Reproduce

Clearly described above

Expected behavior

I would like a way to differentiate listeners; or another way to act upon the picked giphy from the screen itself

(Write what happened. Add screenshots, if applicable.)

Your Environment

Reproducible Demo

ALexanderLonsky commented 7 months ago

Hey @pierroo,

We've concluded that it would be most effective to put the control of event listener management in the hands of the SDK users. Every application has unique requirements and navigation flows. By allowing you to manage event listeners directly, our SDK provides the flexibility to adapt to various use cases and navigation structures. This way, you can subscribe and unsubscribe to events in a manner that aligns seamlessly with your application's lifecycle and user navigation patterns. Managing an internal state listeners within the SDK can lead to unnecessary complexity and potential bugs.

In our example app, I used the following approach to "isolate" views:

  import { useIsFocused } from '@react-navigation/native';
  const addMediaRef = useLatest(addMedia);
  const isFocused = useIsFocused();
  useEffect(() => {  
    if (isFocused) {
      const listener = GiphyDialog.addListener('onMediaSelect', (e) => {
        addMediaRef.current(e.media);
        GiphyDialog.hide();
      });
      return () => {
        console.log('Removing listener');
        listener.remove();
      };
    }
  }, [isFocused, addMediaRef]);

This approach ensures that your event listener is active only when the component is in focus.

Please let me know if that works for you.

P.S. Additionally, you can consider going with this solution

pierroo commented 7 months ago

Hi @ALexanderLonsky ,

Thank you so much for taking the time to answer. This focusRef is finally exactly what we ended up doing, it works well enough for now.

Although another question related would be about the giphy configuration. If I use this in a screen:

GiphyDialog.configure({ mediaTypeConfig: [GiphyContentType.Sticker], })

and a different mediaTypeConfig in another, then the first (or last I'm not sure) will overwrite the other even if I come back later to said screen.

Ideally I would also want this configuration to be related to the screen it's called in only.

ALexanderLonsky commented 7 months ago

For your case, you can reconfigure it similarly: just put the config inside the useEffect:

useEffect(() => {  
    if (isFocused) {
      GiphyDialog.configure({
                mediaTypeConfig: [GiphyContentType.Sticker],
            });
      const listener = GiphyDialog.addListener('onMediaSelect', (e) => {
        addMediaRef.current(e.media);
        GiphyDialog.hide();
      });
      return () => {
        listener.remove();
      };
    }
  }, [isFocused]);

Then you'll have a specific config for a particular view.

github-actions[bot] commented 6 months ago

Hello 👋, this issue has been open for more than a month without a repro or any activity. If the issue is still present in the latest version, please provide a repro or leave a comment within 7 days to keep it open, otherwise it will be closed automatically. If you found a solution or workaround for the issue, please comment here for others to find. If this issue is critical for you, please consider sending a pull request to fix it.