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

[expo-network] Add network state change listeners #28808

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

reichhartd
Copy link
Contributor

Why

This PR introduces a approach to handling network state changes across iOS, Android, and Web platforms in the expo-network module. It adds new methods to manage listeners that respond to changes in network connectivity and network state, improving the module's utility in reactive network management scenarios.

How

To implement this feature, I extended the existing expo-network module to include new methods for adding and removing event listeners.

Test Plan

Tested it for Android in the emulator, and for iOS on my physical device (iPhone 11 Pro).

Checklist

@reichhartd reichhartd requested a review from ide as a code owner May 13, 2024 18:25
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label May 13, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels May 13, 2024
@ide ide requested review from tsapeta and Kudo and removed request for ide May 13, 2024 18:36
packages/expo-network/ios/NetworkModule.swift Outdated Show resolved Hide resolved
packages/expo-network/ios/NetworkModule.swift Outdated Show resolved Hide resolved
Comment on lines 114 to 116
export function removeNetworkStateListener(subscription: Subscription) {
emitter.removeSubscription(subscription);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this? addNetworkStateListener will return a subscription. We have the hook to take care of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented it because some other modules implement it (e.g. expo-clipboard). But the question is whether removeSubscription is needed at all, because addListener returns the remove function.

packages/expo-network/src/Network.ts Outdated Show resolved Hide resolved
export { NetworkState, NetworkStateType };
export { NetworkState, NetworkStateEvent, NetworkStateType };

const emitter = new EventEmitter(ExpoNetwork);
Copy link
Member

Choose a reason for hiding this comment

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

As of SDK 51 we no longer need to use additional EventEmitter to send events. The native module object (ExpoNetwork here) already inherits from a new emitter class that is written in C++, see:

This also solves a long-standing problem with RN's event emitters which are actually global events – event names from different modules can conflict with each other, thus we used to prefix them with something expo-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint, I have adjusted it. However, it would be good if this change was also reflected in the documentation and in the template (npx create-expo-module my-module).

Copy link
Member

@tsapeta tsapeta May 14, 2024

Choose a reason for hiding this comment

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

Yeah, we're planning to do this, however:

  • We want create-expo-module to be compatible with at least one SDK version older than the latest (actually we're going to version it per-SDK but that's another topic).
  • Our docs generation is not yet adjusted to present class inheritance and supported events. We'll also need a new page in the docs that includes all the public APIs exported by the expo package (public and stable APIs from expo-modules-core should be re-exported by expo).

All of those should be solved before SDK 52 though.

@reichhartd reichhartd force-pushed the feat/expo-network-2 branch 2 times, most recently from a8ea357 to 9e02e37 Compare May 14, 2024 12:07
@reichhartd
Copy link
Contributor Author

@alanjhughes @tsapeta Are we ready to merge this? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants