daily-co / react-native-daily-js

https://docs.daily.co/reference/rn-daily-js
BSD 2-Clause "Simplified" License
36 stars 13 forks source link

BUG: Missing removeEventListener when setupGlobals #57

Closed vuptt closed 4 weeks ago

vuptt commented 2 months ago
Screenshot 2024-09-10 at 1 53 29 PM

This code can cause another library crash when they try to use the global variables. Example: @amityco/ts-sdk-react-native

https://github.com/daily-co/react-native-daily-js/blob/react-native-daily-js-releases/src/index.ts#L101

filipi87 commented 2 months ago

Hi @vuptt, thanks for reporting this.

Could you explain how this code might cause another library to crash ? Also, If you can share some code snippets that we can use to reproduce the issue it would be great.

vuptt commented 2 months ago

file: node_modules/@amityco/ts-sdk-react-native/src/client/utils/onOffline.ts

export const onOffline = (callback: () => void) => {
  if (typeof window !== 'undefined' && window.addEventListener) {
    window.addEventListener('offline', callback);
    return () => window.removeEventListener('offline', callback);
  }

  if (typeof document !== 'undefined' && document.addEventListener) {
    document.addEventListener('offline', callback);
    return () => document.removeEventListener('offline', callback);
  }

  //  @TODO: Please update code below to follow our coding conventions and our TS rules
  if (typeof navigator !== 'undefined' && navigator.product === 'ReactNative') {
    // eslint-disable-next-line @typescript-eslint/no-empty-function
    let unsubscribeFn = () => {};

    import('@react-native-community/netinfo').then(NetInfo => {
      unsubscribeFn = NetInfo.addEventListener(state => {
        if (state.isConnected) return;
        callback();
      });
    });

    return () => {
      unsubscribeFn();
    };
  }

  // Handle unsupported environment
  console.error('Unsupported environment');
  return () => console.error('Unsupported environment');
};
filipi87 commented 1 month ago

Hi @vuptt, Just to give you an update, we plan to fix this before our next release. I’ll let you know once it’s ready.

filipi87 commented 4 weeks ago

This issue has been fixed in react-native-daily-js version 0.70.0.