search

darryncampbell / react-native-datawedge-intents

React Native interface for Zebra's DataWedge Intent API
MIT License
43 stars 46 forks source link

Unregister receivers when React Native bundle is reloaded #19

Closed Nikos410 closed 3 years ago

Nikos410 commented 3 years ago

Hey there,

when reloading the React Native bundle the receivers are not unregistered, so we end up with duplicate receivers.

This PR fixes this issue by unregistering the listeners in onCatalystInstanceDestroy (which is called when the React Native bundle is reloaded).

I've also moved the receiver un-registration to a helper method to remove some duplicate code.

darryncampbell commented 3 years ago

Hi, thank you for submitting this pull request.

I have a question, is there a reason for not including the genericReceiver object in the unregisterReceivers method?

Nikos410 commented 3 years ago

Hey Darryn,

the existing code in onHostPause() only unregistered myEnumerateScannersBroadcastReceiver and scannedDataBroadcastReceiver, so I did the same in unregisterReceivers.

If the genericReceiver should also be unregistered I can include it in the new method.

darryncampbell commented 3 years ago

That makes sense. I was not aware of onCatalystInstanceDestroy but there was a similar issue raised a few months back on my demo app, i.e. it was registering a broadcast receiver multiple times during development (hot reload). I "fixed" the issue with https://github.com/darryncampbell/DataWedgeReactNative/commit/79d5325f486bae3c4cd7c45486f7ded189ca5a9c but it sounds like a superior fix would have been to do something similar to what you have done with onCatalystInstanceDestroy.

That was where my question was coming from... if you do not include genericReceiver in onCatalystInstanceDestroy, I think you would still end up with duplicate receivers, at least for genericReceiver.

Sorry I don't have more time to dedicate to this and answer my question but I have tested your change with my demo app & it all works so I will merge this PR.

Thanks again.

Nikos410 commented 3 years ago

Thank you for your quick response @darryncampbell ! Could you publish the latest version on npm?

darryncampbell commented 3 years ago

Sure, done just now: https://www.npmjs.com/package/react-native-datawedge-intents/v/0.1.7