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

Fix for #6 in useNavigationEvent #8

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@slorber

slorber commented Nov 18, 2018

No description provided.

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Nov 26, 2018

I'm tempted to merge this and cut a release because it does fix the issue.

But I'm afraid this is more complexity than desired.. is the ref to the handler really needed? Maybe we need to change things on the @react-navigation/core side, but I feel that useEffect should be sufficient for this use case

@slorber

This comment has been minimized.

slorber commented Nov 27, 2018

Yes it is necessary because otherwise if the key does not change, the useEffect closure will capture the initial handleEvt fn, and that initial fn will be called by the navigation listeners. We really want to ensure that the last handleEvt fn provided by the user is called.

Let's consider that the key will not change.
If user does useNavigationEvent(bool ? handler1: handler2), only one of the handler will keep being called because it was captured in the useEffect closure, even if the bool value change over time, which is dangerous and unexpected from an user point of view.

@slorber

This comment has been minimized.

slorber commented Nov 27, 2018

The recommended way is to unsub/resub as it's generally fast and does not produce side effects.
https://twitter.com/dan_abramov/status/1066374847427682305

However for react-navigation, on (re)subscribe you might get a "focus" event firing synchronously. So if we want to simplify this implementation, we must remove that react-navigation-core behavior first, which may be a wanted. I don't know the details of why we need that event firing synchrously on subscribe but you or @brentvatne are likely the best to know that ;)

@gaearon

This comment has been minimized.

gaearon commented Nov 27, 2018

However for react-navigation, on (re)subscribe you might get a "focus" event firing synchronously.

That sounds like the root of the problem that could be solved. Ideally resubscription shouldn't trigger side effects.

With the approach in this PR, I don't understand why [navigation.state.key] is the second argument. If you wanted a stable listener, why not just [] and always use the same one? Otherwise you'd get the same "side effects on resubscription" problems when navigation.state.key changes. Or is that desirable?

Another minor caveat is that useEffect has been reported to be buggy in RN and fired way too late. Don't know if that got fixed.

@slorber

This comment has been minimized.

slorber commented Nov 27, 2018

Thanks for your review @gaearon

That sounds like the root of the problem that could be solved. Ideally resubscription shouldn't trigger side effects.

Agree with that, but I suspect there's a good reason to have that side effect that I'm not aware of.

[navigation.state.key] is the inputs because we actually want to resubscribe when the navigation state change. Not sure to exactly know why and @ericvicenti probably knows this better, but I suspect this means the navigation passed through props is not the same as before (ie, a different listener list). The key is generally stable during the lifecycle of a react-navigation screen and should change on screen replace.

I wonder if [] could not be used instead, because I suspect react-navigation does remount the screen component on state.key change (can someone confirm?). If the key can't change over the lifecycle of a screen comp, there's probably no need to use it as input.

Yes I've seen useEffect is not working properly (just released https://github.com/slorber/react-native-animation-hooks which has the useEffect issue), but hopefully this implementation is supposed to work when useEffect will work :)

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Nov 28, 2018

Thanks for the advice @gaearon!

I'm looking at the events code and I can't see how/why/when we would get a synchronous focus event when (re)subscribing. Unless you're referring to the actual normal focus event, which might be happening around the same time as the re-subscription? Do you have an easy way to repro this behavior?

When we call addEventListener, it starts listening to the events of the current route, which is identified as navigation.state.key. In almost all circumstances, it will never change. But, if it does change for some reason, it would make sense for our listener to unsubscribe and re-subscribe to the new route. Which is why I think [navigation.state.key] makes sense as the second argument.

(When I first tried using the second argument with state.key, I think I saw some bad and confusing behavior. But I can't really remember what was going wrong, so if this second argument works for anyone else, lets just move forward with it until a more specific bug emerges)

I prototyped this stuff on web, so I haven't had too much time with hooks on RN. Once the useEffect thing is sorted out, I'll move over my RN project to use ReactNavHooks

@gaearon

This comment has been minimized.

gaearon commented Nov 28, 2018

I'm looking at the events code and I can't see how/why/when we would get a synchronous focus event when (re)subscribing. Unless you're referring to the actual normal focus event, which might be happening around the same time as the re-subscription? Do you have an easy way to repro this behavior?

I don't personally have any knowledge about resubscription behavior. I've never run this code — I'm here from a twitter thread with @slorber.

I was referring to this comment (#8 (comment)) which says that it causes focus to fire.

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Nov 28, 2018

Yeah I figured you had been summoned here from Twitter 😂👋 It was a poorly-addressed "you're".

@slorber, let me know what happens in your code when you try putting the route key in the second argument of useEffect. Hopefully it will work fine!

@slorber

This comment has been minimized.

slorber commented Nov 29, 2018

@ericvicenti if I put the route key in my impl (which I did in this PR) it works fine, much better than the current implementation. But I encounter another weird bug afterward where navigation events stop firing completely (as far as I remember, this is true for both your impl and mine, using your react navigation web example projet as reference, so it does not look like a regression)

I'm looking at the events code and I can't see how/why/when we would get a synchronous focus event when (re)subscribing. Unless you're referring to the actual normal focus event, which might be happening around the same time as the re-subscription? Do you have an easy way to repro this behavior?

Yes, I probably was wrong and this focus event is not totally synchronous but happens just after subscribe.

This behavior lead to infinite loops when using an arrow function (see react-navigation/react-navigation#5058)

<NavigationEvents onDidFocus={() => this.setState({})} />

The setState trigger a new render, and in the next update, as the closure might have changed, we have to unsub/resub, but we are still in the screen appearance phase, so the onDidFocus event keeps firing again and again as we unsub/resub.

Using a stable listener did not have that bug, because former implementation did not unsub/resub when the callback is stable.

The fix to always register a stable listener on mount was merged and fixes the issue:
react-navigation/react-navigation-core#17

My PR here is supposed to also ensure this issue does not happen with hooks when using an arrow function listener ;)

The user might to something like this which would trigger the same issue:

useNavigationEvents(event => {
  if ( event.type = "focus" ) setIsFocusedInState(true);
})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment