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

Prevent outdated connectivity info from updating the online status #58

Conversation

birgernass
Copy link

Sometimes the DetectNetwork#init method (called in the constructor) updates the online status to 'unknown' after one of the event listeners already updated the online status with the correct reachability. This happens especially on iOS and prevents all queued actions from being fired.

@sorodrigo
Copy link
Owner

sorodrigo commented Oct 30, 2017

Hey! Thanks for bringing this up. I think a more elegant solution would be to await this._init() on the constructor. That would prevent init executing after the events the event listeners trigger.

@birgernass
Copy link
Author

I don't think that we can await in the constructor. I'm also concerned that we would set the event listeners too late and that we would miss the first change and connectionChange events, resulting in the status being unknown again. But I like to be convinced of the contrary and I'm totally open to different solutions.

@sorodrigo
Copy link
Owner

I think to remember that the events trigger more than once, that's why we check if they trigger a change before dispatching. If this is true, I think awaiting in the constructor won't be that problematic. Maybe we could test if this works and if not continue with another solution.

@birgernass
Copy link
Author

They do trigger more than once, but not necessarily on app start. They trigger once on startup. At least in my experience. After that, the network connectivity or the app state (foreground/background) have to change. But when they don't, the state sometimes remains 'unknown'. As I said, this doesn't happen every time, but often enough to be reproducible for me.

@sorodrigo
Copy link
Owner

I get your point, I think that a lot of people suffered this issue without noticing. Thanks for identifying it and providing a fix! 🎉

@sorodrigo sorodrigo merged commit ea808c6 into sorodrigo:develop Nov 4, 2017
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.

2 participants